* Version WebKit nightly r188586 * SUMMARY A Breakpoint that pauses the Debugger prevents me from reloading the current page. * STEPS TO REPRODUCE 1. Inspect any page 2. Set a breakpoint 3. Wait until the breakpoint pauses the Debugger 3. Cmd+R to reload => You can't reload the page * EXPECTATION Page reload works. FYI: Looking at https://bugs.webkit.org/show_bug.cgi?id=62631 it seems a LayoutTest (LayoutTests/inspector/debugger/debugger-reload-on-pause.html) got removed. At least I can't find the test anymore.
<rdar://problem/22339835>
(In reply to comment #0) > * Version > WebKit nightly r188586 > > * SUMMARY > A Breakpoint that pauses the Debugger prevents me from reloading the current > page. > > * STEPS TO REPRODUCE > 1. Inspect any page > 2. Set a breakpoint > 3. Wait until the breakpoint pauses the Debugger > 3. Cmd+R to reload > => You can't reload the page > > * EXPECTATION > Page reload works. > > FYI: Looking at https://bugs.webkit.org/show_bug.cgi?id=62631 it seems a > LayoutTest (LayoutTests/inspector/debugger/debugger-reload-on-pause.html) > got removed. At least I can't find the test anymore. This was from before the fork, and most of those tests were deprecated and eventually deleted. But, you can probably fix it up for our modern WK times.
<rdar://problem/19147805>
So I'm seeing a similar issue. It seems, when we reload, it just "Resumes" and hits the next breakpoint. There are likely a few issues here, but lets start with that issue.
Created attachment 261172 [details] [PATCH] Work in Progress This work in progress fixes reloading through the inspector (PageAgent::reload). However, if you reload the page from Safari it only continues until the next breakpoint (the existing bad behavior I'm seeing).
Created attachment 261196 [details] [PATCH] Proposed Fix This seems like a much better fix. It'll handle reloading/navigation from Safari or Inspector. If reloading/navigating when paused, you will resume and not pause until the load is complete. If not paused, reloading/navigating allows for pausing, for late page JavaScript like unload/pagehide handlers (which is what I would expect if I set breakpoints there).
Attachment 261196 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:92: The parameter name "skipAllPauses" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
* TEST <button id="x">Click Me</button> <script> document.getElementById('x').addEventListener('click', function() { debugger; }); window.onunload = function() { debugger; } window.onpagehide = function() { debugger; } </script> * NOTES Comparing to other browsers on a test case like this: - Chrome - paused in click handler + reload => no pauses - no pause + reload => no pauses - Firefox - paused in click handler + reload => no pauses - no pause + reload => pause in pagehide/unload - Safari - paused in click handler + reload => no pauses - no pause + reload => pause attempted in pagehide/unload (CRASH in DocumentWriter at the moment, bug 80427)
Created attachment 261197 [details] [PATCH] Proposed Fix
Created attachment 261200 [details] [PATCH] Proposed Fix (remove old resume())
Comment on attachment 261200 [details] [PATCH] Proposed Fix (remove old resume()) View in context: https://bugs.webkit.org/attachment.cgi?id=261200&action=review > Source/WebCore/loader/FrameLoader.cpp:2988 > + page->inspectorController().resumeForNavigation(); I am all in favor of more InspectorInstrumentation and fewer hardcoded calls to the inspector controller. willStartProvisionalLoad? I know that's obscure to someone unfamiliar with loader code, but that's what the 'event' is. > Source/WebCore/loader/FrameLoader.cpp:3419 > + m_frame.page()->inspectorController().didNavigate(); Why not hook this from InspectorInstrumentation::didCommitLoadImpl instead?
Created attachment 261224 [details] [PATCH] Proposed Fix
Comment on attachment 261224 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261224&action=review It looks good, but I realized that we forgot about one case: : if the provisional load is *not* committed, how do we ensure that suppressAllPauses gets cleared? This seems to mostly happen under FrameLoader::stopAllLoaders, i.e., because of user cancellation, X-Frame-Options, or other things. I think we might be able to use existing instrumentation hooks to detect non-commits. > Source/JavaScriptCore/ChangeLog:14 > + (Inspector::InspectorDebuggerAgent::setSkipAllPauses): Nit: change log needs update.
Created attachment 261242 [details] [PATCH] Proposed Fix This switches to: frameStartedLoading/frameStoppedLoading/frameNavigated event times.
Comment on attachment 261242 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261242&action=review r=me, looks great. > Source/JavaScriptCore/ChangeLog:17 > + Provide a way to supress pauses. Nit: suppress
Created attachment 261272 [details] [PATCH] For Landing
Comment on attachment 261272 [details] [PATCH] For Landing Clearing flags on attachment: 261272 Committed r189834: <http://trac.webkit.org/changeset/189834>