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.
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.
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.
(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?
We might be able to get it working out-of-process with WebKit1 too. I am focusing on a WK2 solution at the moment.
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.
Created attachment 237316 [details] Proposed fix
Created attachment 237317 [details] Proposed fix (rebased)
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.
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.
Created attachment 237321 [details] Proposed fix (with minification)
This has been made unnecessary by the fix to https://bugs.webkit.org/show_bug.cgi?id=135120
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).
Hi, This bug was pointed to me by pecoraro@apple.com 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
(In reply to comment #13) > Hi, > > This bug was pointed to me by pecoraro@apple.com 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.
>> 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.
(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
(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.
(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.
(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.