Bug 148174 - Web Inspector: Paused Debugger prevents page reload
Summary: Web Inspector: Paused Debugger prevents page reload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-19 01:06 PDT by Tobias Reiss
Modified: 2015-09-21 14:07 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Work in Progress (8.47 KB, patch)
2015-09-14 21:58 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (7.73 KB, patch)
2015-09-15 09:49 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (7.72 KB, patch)
2015-09-15 09:53 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (remove old resume()) (7.95 KB, patch)
2015-09-15 10:11 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (13.78 KB, patch)
2015-09-15 12:17 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (12.63 KB, patch)
2015-09-15 14:55 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff
[PATCH] For Landing (12.62 KB, patch)
2015-09-15 17:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Reiss 2015-08-19 01:06:56 PDT
* 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.
Comment 1 Radar WebKit Bug Importer 2015-08-19 01:07:27 PDT
<rdar://problem/22339835>
Comment 2 BJ Burg 2015-08-19 15:27:59 PDT
(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.
Comment 3 Timothy Hatcher 2015-08-26 10:14:28 PDT
<rdar://problem/19147805>
Comment 4 Joseph Pecoraro 2015-09-14 21:46:37 PDT
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.
Comment 5 Joseph Pecoraro 2015-09-14 21:58:35 PDT
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).
Comment 6 Joseph Pecoraro 2015-09-15 09:49:04 PDT
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).
Comment 7 WebKit Commit Bot 2015-09-15 09:51:21 PDT
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.
Comment 8 Joseph Pecoraro 2015-09-15 09:53:05 PDT
* 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)
Comment 9 Joseph Pecoraro 2015-09-15 09:53:49 PDT
Created attachment 261197 [details]
[PATCH] Proposed Fix
Comment 10 Joseph Pecoraro 2015-09-15 10:11:22 PDT
Created attachment 261200 [details]
[PATCH] Proposed Fix (remove old resume())
Comment 11 BJ Burg 2015-09-15 10:30:46 PDT
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?
Comment 12 Joseph Pecoraro 2015-09-15 12:17:45 PDT
Created attachment 261224 [details]
[PATCH] Proposed Fix
Comment 13 BJ Burg 2015-09-15 13:07:57 PDT
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.
Comment 14 Joseph Pecoraro 2015-09-15 14:55:34 PDT
Created attachment 261242 [details]
[PATCH] Proposed Fix

This switches to: frameStartedLoading/frameStoppedLoading/frameNavigated event times.
Comment 15 BJ Burg 2015-09-15 16:19:08 PDT
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
Comment 16 Joseph Pecoraro 2015-09-15 17:46:13 PDT
Created attachment 261272 [details]
[PATCH] For Landing
Comment 17 WebKit Commit Bot 2015-09-15 18:03:50 PDT
Comment on attachment 261272 [details]
[PATCH] For Landing

Clearing flags on attachment: 261272

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