Bug 153193 - Reproducible "Unhandled web process message 'WebUserContentController:AddUserScripts'" and friends
Summary: Reproducible "Unhandled web process message 'WebUserContentController:AddUser...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
Keywords: InRadar
: 152386 (view as bug list)
Depends on:
Reported: 2016-01-17 00:26 PST by Tim Horton
Modified: 2016-09-05 13:33 PDT (History)
9 users (show)

See Also:

API test that reproduces the problem (and asserts/crashes later too) (3.43 KB, patch)
2016-01-17 00:26 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (10.20 KB, patch)
2016-01-17 01:26 PST, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2016-01-17 00:26:40 PST
Created attachment 269188 [details]
API test that reproduces the problem (and asserts/crashes later too)

Steps to Reproduce:

1. Apply the attached test patch and run the API test.


The WebPageProxy constructor assumes that if the process is already running, it can add itself to the existing WebUserContentController(Proxy) and all will be well.

However, if the API client constructs a different WKUserContentController for two views, and forces them both into the same process (by setting the process cap to 1), when WebPageProxy's constructor goes to use the existing WebUserContentController, it sends a message with a WebUserContentController ID that doesn't exist yet on the WebProcess side (because createWebPage, which usually brings it up, hasn't happened yet), and we lose the message.

This also means that we'll crash and burn horribly if we later try to remove e.g. a UserScriptHandler that was lost in the lost Add messages.
Comment 1 Tim Horton 2016-01-17 00:27:35 PST
I imagine one could do something similar with the visited link store, since the same assumption is made about it.
Comment 2 Radar WebKit Bug Importer 2016-01-17 00:31:31 PST
Comment 3 Tim Horton 2016-01-17 01:02:03 PST
The fix is not super obvious.

I considered moving the offending block:

if (m_process->state() == WebProcessProxy::State::Running) {
    if (m_userContentController)

to the end of initializeWebPage, after CreateWebPage is sent, but then in the reattachToWebProcess case, we'll end up doing the work twice (because reattachToWebProcess calls processDidFinishLaunching if the process is up, which ALSO includes the above block).
Comment 4 Tim Horton 2016-01-17 01:26:37 PST
Created attachment 269189 [details]
Comment 5 WebKit Commit Bot 2016-01-17 01:27:50 PST
Attachment 269189 [details] did not pass style-queue:

ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:212:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:213:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:214:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 5 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Darin Adler 2016-01-18 15:32:00 PST
Comment on attachment 269189 [details]

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:367
> +    , m_needsInitializationAfterProcessLaunch(false)

Should initialize this in the header instead of here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:802
> +void WebPageProxy::initializeWebPageAfterProcessLaunch()

I think you should call this finishInitializingWebPageAfterProcessLaunch to make the relationship with initializeWebPage clearer. The current name makes it sound like this function will call initializeWebPage.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:808
> +    m_needsInitializationAfterProcessLaunch = false;

I think it’s worth refactoring so the logic is more contained in a single function. Here’s one idea (less important than my other review feedback, I think). In WebPageProxy::initializeWebPage:

    m_needsToFinishInitializingWebPageAfterProcessLaunch = true;

Then, in finishInitializingWebPageAfterProcessLaunch:

    if (!m_needsToFinishInitializingWebPageAfterProcessLaunch)
    if (m_process->state() != WebProcessProxy::State::Running)
    m_needsToFinishInitializingWebPageAfterProcessLaunch = false;

    if (m_userContentController)

Then in processDidFinishLaunching another unconditional call:


This seems to cut down the surface area a bit and make it slightly more foolproof.

> Source/WebKit2/UIProcess/WebPageProxy.h:357
> +    void initializeWebPageAfterProcessLaunch();

Should be private.
Comment 7 Tim Horton 2016-01-22 14:26:35 PST
Comment 8 Michael Catanzaro 2016-09-05 13:33:02 PDT
*** Bug 152386 has been marked as a duplicate of this bug. ***