Bug 153193 – Reproducible "Unhandled web process message 'WebUserContentController:AddUserScripts'" and friends

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

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.

Notes:

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.

[Tim Horton]

I imagine one could do something similar with the visited link store, since the same assumption is made about it.

[Radar WebKit Bug Importer]

<rdar://problem/24222034>

[Tim Horton]

The fix is not super obvious.

I considered moving the offending block:

if (m_process->state() == WebProcessProxy::State::Running) {
    if (m_userContentController)
        m_process->addWebUserContentControllerProxy(*m_userContentController);
    m_process->addVisitedLinkStore(m_visitedLinkStore);
}

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).

[Tim Horton]

Created attachment 269189 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

Comment on attachment 269189 [details]
Patch

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;
    finishInitializingWebPageAfterProcessLaunch();

Then, in finishInitializingWebPageAfterProcessLaunch:

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

    if (m_userContentController)
        m_process->addWebUserContentControllerProxy(*m_userContentController);
    m_process->addVisitedLinkStore(m_visitedLinkStore);

Then in processDidFinishLaunching another unconditional call:

    finishInitializingWebPageAfterProcessLaunch();

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.

[Tim Horton]

http://trac.webkit.org/changeset/195479

[Michael Catanzaro]

*** Bug 152386 has been marked as a duplicate of this bug. ***