Bug 148174 – Web Inspector: Paused Debugger prevents page reload

• Component: Web Inspector
• Status: RESOLVED FIXED

[Tobias Reiss]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/22339835>

[Blaze Burg]

(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.

[Timothy Hatcher]

<rdar://problem/19147805>

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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).

[Joseph Pecoraro]

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).

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

* 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)

[Joseph Pecoraro]

Created attachment 261197 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 261200 [details]
[PATCH] Proposed Fix (remove old resume())

[Blaze Burg]

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?

[Joseph Pecoraro]

Created attachment 261224 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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.

[Joseph Pecoraro]

Created attachment 261242 [details]
[PATCH] Proposed Fix

This switches to: frameStartedLoading/frameStoppedLoading/frameNavigated event times.

[Blaze Burg]

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

[Joseph Pecoraro]

Created attachment 261272 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 261272 [details]
[PATCH] For Landing

Clearing flags on attachment: 261272

Committed r189834: <http://trac.webkit.org/changeset/189834>