Bug 135830 – Promise reactions in the Inspector page do not run when the debugger is paused on the inspected page

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Brian Burg]

We are starting to use promises more in the inspector, so this is a fairly big problem.

I was under the impression that promise microtasks eventually run as a JSGlobalObjectTask through ScriptExecutionContext::postTask, so it's interesting that the inspector's Promises don't react when the debugger is paused.

[Brian Burg]

Diagnosis;

Promise reactions create JSGlobalObjectTasks to run reactions, which are enqueued via Document::postTask. This in turn uses callOnMainThread() to get onto the main thread. Unfortunately, callOnMainThread uses a single global variable ('callbacksPaused') to pause callbacks while the debugger is paused. So, the second-level debugger's tasks will not get called on the main thread until the nested event loop shuts down and the debugger unpauses callbacks on the main thread (called from PageScriptDebugServer::setJavaScriptPaused).

Potential fixes;

1. Make the inspector multi-process, so we can use global variables this way.

2. Segregate main thread callbacks by PageGroup (unit of JavaScript being paused). This would be unpleasant, and change how tasks from different pages are interleaved unless we use a round-robin approach or something.

[Timothy Hatcher]

Moving the Inspector out-of-process is a task I am looking at this week. However, it will still be in-process for WebKit1 apps. So we likely need to fix this in WebCore too.

[Sam Weinig]

(In reply to comment #2)
> Moving the Inspector out-of-process is a task I am looking at this week. However, it will still be in-process for WebKit1 apps. So we likely need to fix this in WebCore too.

Out of curiosity, why keep it in-process for WebKit1 apps?

[Timothy Hatcher]

We might be able to get it working out-of-process with WebKit1 too. I am focusing on a WK2 solution at the moment.

[Brian Burg]

To fix this in WebCore we would need a per-Page function queue, mutex, and have multiple callers to scheduleDispatchOnMainThread. I don't know that code well enough to tell if this is easy or hard (due to some subtle invariants). There are a lot of uses of callOnMainThread throughout WebCore and many of those don't have a Page context, so only some of the callbacks would be tied to a Page. So we might have to think carefully about what other parts of the code should/should not be suspended.

I wrote a quick workaround which defines WebInspector.Promise = RSVP.Promise, and uses WebInspector.Promise everywhere. RSVP (https://github.com/tildeio/rsvp.js) is an MIT-licensed promise library in JS. It should be easy to remove this workaround after we land a fix in WebCore or make progress towards multi-process Inspector.

[Brian Burg]

Created attachment 237316 [details]
Proposed fix

[Brian Burg]

Created attachment 237317 [details]
Proposed fix (rebased)

[Brian Burg]

Comment on attachment 237317 [details]
Proposed fix (rebased)

If we go forward with the workaround patch, then the patch also needs to add some lines to copy-user-interface-resources.pl for copying the LICENSE file appropriately and minifying the script.

[Timothy Hatcher]

Comment on attachment 237317 [details]
Proposed fix (rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=237317&action=review

> Source/WebInspectorUI/UserInterface/External/RSVP/LICENSE:1
> +Copyright (c) 2014 Yehuda Katz, Tom Dale, Stefan Penner and contributors

I'll file an internal OSS request for RSVP.

> Source/WebInspectorUI/UserInterface/Main.html:145
> +    <script src="External/RSVP/rsvp.js"></script>

copy-user-interface-resources.pl needs modified to do the right thing when resources are combined. You can copy how Esprima.js is handled.

[Brian Burg]

Created attachment 237321 [details]
Proposed fix (with minification)

[Brian Burg]

This has been made unnecessary by the fix to https://bugs.webkit.org/show_bug.cgi?id=135120

[Csaba Osztrogonác]

Comment on attachment 237321 [details]
Proposed fix (with minification)

Cleared review? from attachment 237321 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Chris Vienneau]

Hi,

This bug was pointed to me by [e-mail] when i complained about the following behavior:

Here are the simple steps to reproduce it,
1.       Launch WinCairo and go to google.com
2.       Open Web inspector and open the script source of any .js script
3.       Set a breakpoint anywhere
4.       Reload the web page
 
What we see is the spinner spinning and never the script source. However, if one presses continue-script-execution from the debugger controls, we get the view back.

We see the same behavior in our application, both WinCairo and our app are WebKit1 based so we don't benefit from the multiple processes.  I attempted to apply the last patch to use the WebInspector.Promise instead of the default one.  I believe i've applied it correctly but my behavior doesn't change at all.

Are there any suggestions? Perhaps the changes here would not work against my version (188436). Do you suspect a different problem?

Thank for any help

Chris

[Blaze Burg]

(In reply to comment #13)
> Hi,
> 
> This bug was pointed to me by [e-mail] when i complained about the
> following behavior:
> 
> Here are the simple steps to reproduce it,
> 1.       Launch WinCairo and go to google.com
> 2.       Open Web inspector and open the script source of any .js script
> 3.       Set a breakpoint anywhere
> 4.       Reload the web page
>  
> What we see is the spinner spinning and never the script source. However, if
> one presses continue-script-execution from the debugger controls, we get the
> view back.
> 
> We see the same behavior in our application, both WinCairo and our app are
> WebKit1 based so we don't benefit from the multiple processes.  I attempted
> to apply the last patch to use the WebInspector.Promise instead of the
> default one.  I believe i've applied it correctly but my behavior doesn't
> change at all.
> 
> Are there any suggestions? Perhaps the changes here would not work against
> my version (188436). Do you suspect a different problem?
> 
> Thank for any help
> 
> Chris

It's very likely that we have added more uses of Promise in the inspector codebase since the patch was written. So, you'll need to find those and replace with the WebInspector.Promise shim.

I think it still might be worth committing this shim to trunk so that Inspector is actually usable with a local WK1 view (mostly, for Windows). If you get it to work on your branch, I can do the additional build system work to make WebInspector.Promise only use the RSVP library for WK1 ports.

[Chris Vienneau]

>> you'll need to find those and replace with the WebInspector.Promise shim.
Yea, sorry i didn't mention that but i had already attempted that.  I applied the patch manually by looking at the diffs, and searching and replacing for appropriate spots where "Promise" should become "WebInspector.Promise".  It is possible that i have made some mistake, but I believe i applied it all as intended.

[Chris Vienneau]

(In reply to comment #15)
> >> you'll need to find those and replace with the WebInspector.Promise shim.
> Yea, sorry i didn't mention that but i had already attempted that.  I
> applied the patch manually by looking at the diffs, and searching and
> replacing for appropriate spots where "Promise" should become
> "WebInspector.Promise".  It is possible that i have made some mistake, but I
> believe i applied it all as intended.

My effected files were:
WebInspectorUI/Scripts/copy-user-interface-resources.pl
WebInspectorUI/UserInterface/Base/DOMUtilities.js
WebInspectorUI/UserInterface/Base/Test.js
WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js
WebInspectorUI/UserInterface/Controllers/DebuggerManager.js
WebInspectorUI/UserInterface/Controllers/ReplayManager.js
WebInspectorUI/UserInterface/External/RSVP/LICENSE
WebInspectorUI/UserInterface/External/RSVP/rsvp.js
WebInspectorUI/UserInterface/Main.html
WebInspectorUI/UserInterface/Models/CSSStyleSheet.js
WebInspectorUI/UserInterface/Models/ReplaySession.js
WebInspectorUI/UserInterface/Models/Resource.js
WebInspectorUI/UserInterface/Models/Script.js
WebInspectorUI/UserInterface/Models/SourceCode.js
WebInspectorUI/UserInterface/Models/SourceMapResource.js
WebInspectorUI/UserInterface/Models/WrappedPromise.js
WebInspectorUI/UserInterface/Protocol/InspectorBackend.js
WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js
WebInspectorUI/UserInterface/Views/TabBar.js

[Blaze Burg]

(In reply to comment #15)
> >> you'll need to find those and replace with the WebInspector.Promise shim.
> Yea, sorry i didn't mention that but i had already attempted that.  I
> applied the patch manually by looking at the diffs, and searching and
> replacing for appropriate spots where "Promise" should become
> "WebInspector.Promise".  It is possible that i have made some mistake, but I
> believe i applied it all as intended.

I would attach a debugger to see where the inspector/inspected page is getting hung. Unfortunately, pretty much anything that indirectly uses callOnMainThread can cause the inspector to hang, and these are used in many places these days. Known issues might be data: blob urls (used by inspector for images), requestAnimationFrame, the FileStream API, and most media things (audio / media source / etc).

There is unfortunately not a lot that can be done, IMO. Ideally these things would be ActiveDOMObjects and suspend properly when the debugger pauses, but there have been many bugs in that part of the codebase that mistakenly allow some DOM features to execute while script is paused.

In the past, we got around the WK1 hang by using a remote Web Inspector, but that has never existed for Windows.

[Chris Vienneau]

(In reply to comment #17)
> In the past, we got around the WK1 hang by using a remote Web Inspector, but
> that has never existed for Windows.

My port does have remote web inspector setup, I just tried it out in this scenario and found that it does function properly.  I think this will be an acceptable workaround for us as well.

[Blaze Burg]

(In reply to comment #18)
> (In reply to comment #17)
> > In the past, we got around the WK1 hang by using a remote Web Inspector, but
> > that has never existed for Windows.
> 
> My port does have remote web inspector setup, I just tried it out in this
> scenario and found that it does function properly.  I think this will be an
> acceptable workaround for us as well.

OK, marking WONTFIX again. Joe has proposed removing local WK1 inspector support since the underlying issue is unlikely to be addressed.