Bug 213112 - Remove WTF::setMainThreadCallbacksPaused
Summary: Remove WTF::setMainThreadCallbacksPaused
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-11 21:55 PDT by Geoffrey Garen
Modified: 2020-06-23 17:28 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.32 KB, patch)
2020-06-11 21:59 PDT, Geoffrey Garen
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (4.29 KB, patch)
2020-06-23 16:41 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2020-06-11 21:55:17 PDT
Remove WTF::setMainThreadCallbacksPaused
Comment 1 Geoffrey Garen 2020-06-11 21:59:21 PDT
Created attachment 401706 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-06-11 22:32:25 PDT
Comment on attachment 401706 [details]
Patch

What impact will removing this have on JS debugging? It's been there since 2010.
Comment 3 Tim Horton 2020-06-11 23:04:41 PDT
Comment on attachment 401706 [details]
Patch

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

> Source/WTF/ChangeLog:16
> +        even queue. That should suspend JavaScript-visible stuff without

s/even/event/

Also... if we can really do this, why don't we use this for JS-synchronous-but-ideally-not-engine-synchronous things like alert()?
Comment 4 Geoffrey Garen 2020-06-12 09:38:18 PDT
> Also... if we can really do this, why don't we use this for
> JS-synchronous-but-ideally-not-engine-synchronous things like alert()?

Use... what? :P

alert() still needs to run a nested run loop or a sync IPC because it's not possible to tear down the whole stack and return to the main run loop -- that's not currently a feature of C++ or of the JS engine.

Also, the debugger intends to allow a little more continuing execution than alert(). For example, the debugger intends to allow scrolling, selection, re-layout, and nested JS execution, and alert() does not.
Comment 5 Tim Horton 2020-06-12 10:03:32 PDT
Aha! Ok, makes sense.
Comment 6 Geoffrey Garen 2020-06-12 10:18:12 PDT
> What impact will removing this have on JS debugging? It's been there since
> 2010.

This code is all about how WebCore behaves when stopped at a breakpoint.

Some edge case oddities will be fixed. For example, large images will be able to paint now.

If I've overlooked something, some JS code will still run. So, you won't be as stopped as you wanted to be. The good news is, we can find and fix that kind of bug by migrating the code to the explicit JS event queue, which is much more reliable than just pausing all scheduled main thread code, including non-JS code.

All of this being said, EWS says I did break a WK2 inspector test. Time for some fun debugging.
Comment 7 Geoffrey Garen 2020-06-12 16:38:26 PDT
> All of this being said, EWS says I did break a WK2 inspector test. Time for
> some fun debugging.

Actually, that inspector test was broken by r262904, not this patch. So, back up for review.
Comment 8 Geoffrey Garen 2020-06-23 16:38:41 PDT
Re-testing rdar://problem/14133001 still passes.
Comment 9 Geoffrey Garen 2020-06-23 16:41:56 PDT
Created attachment 402607 [details]
Patch for landing
Comment 10 EWS 2020-06-23 17:27:48 PDT
Committed r263432: <https://trac.webkit.org/changeset/263432>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402607 [details].
Comment 11 Radar WebKit Bug Importer 2020-06-23 17:28:17 PDT
<rdar://problem/64673032>