Bug 52248 – Back/forward list recovery after a WebProcess crash is crashy itself.

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Back/forward list recovery after a crash is crashy itself.

If the WebProcess crashes and we restore the back/forward list to the new session, the UIProcess and WebProcess are not in agreement about the list and this leads to crashes downstream.

Shortly I'll attach a patch that:
-Relaunchs the WebProcess on goBack/goForward/goToItem (and reload() for good measure)
-Sends session state over to the new WebPage in the new WebProcess as part of the creation parameters.
-Optionally reloads the current history item to get something in the page after recovery.

In radar mostly as <rdar://problem/8837307>.

The "WebPageProxy::reload() should relaunch the process" piece is in radar as <rdar://problem/8637038>

[Brady Eidson]

Created attachment 78595 [details]
Patch v1

[WebKit Review Bot]

Attachment 78595 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/Shared/Sessi..." exit_code: 1
WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:83:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:44:  The parameter name "itemID" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 78595 [details] did not build on win:
Build output: http://queues.webkit.org/results/7463100

[Early Warning System Bot]

Attachment 78595 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7486009

[Darin Adler]

Comment on attachment 78595 [details]
Patch v1

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

Looks like this failed to build on Windows.

> WebKit2/Shared/SessionState.cpp:62
> +    // This might change.

This comment is cryptic. I think what you mean to say is:

    // Because this might change later, callers should use this instead of calling list().isEmpty() directly themselves.

> WebKit2/UIProcess/WebPageProxy.cpp:212
> +    reattachToWebProcess(true);

The boolean here is not good. It’s unclear what “true” means. I’d rather see an enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:238
> -    process()->send(Messages::WebProcess::CreateWebPage(m_pageID, creationParameters()), 0);
> +    process()->send(Messages::WebProcess::CreateWebPage(m_pageID, creationParameters(reloadCurrentSessionHistoryItem)), 0);

Instead of sending a boolean, could we just send a second message to trigger the reloading?

> WebKit2/UIProcess/WebPageProxy.cpp:312
> +        reattachToWebProcess(false);

The boolean here is not good. It’s unclear what “false” means. I’d rather see an enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:322
> +        reattachToWebProcess(false);

The boolean here is not good. It’s unclear what “false” means. I’d rather see an enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:365
> +        reattachToWebProcess(true);

The boolean here is not good. It’s unclear what “true” means. I’d rather see an enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:377
> +    if (!isValid()) {
> +        reattachToWebProcessWithItem(m_backForwardList->forwardItem());
>          return;
> +    }

Might be over-engineering to support this. How did you test it?

> WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:74
> +void WebBackForwardListProxy::setHighestItemIDFromUIProcess(uint64_t itemID)

Do we have a race condition here? What if an ID is created before receiving the new highest item?

> WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:84
> +    uniqueHistoryItemID = itemID;
> +
> +    // If the highest used ID is even, increment it to the next odd ID as that is what should be used
> +    // for items generated by the WebProcess. 
> +    if (uniqueHistoryItemID % 2 == 0)
> +        ++uniqueHistoryItemID;

I think this would read better if you set uniqueHistoryItemID only once. Maybe like this:

    if (itemID % 2)
        uniqueHistoryItemID = itemID;
    else
        uniqueHistoryItemID = itemID + 1;

Or with a function:

    static uint64_t makeOdd(uint64_t number)
    {
        return number + number % 2;
    }

And:

    uniqueHistoryItemID = makeOdd(itemID);

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 78595 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78595&action=review
> 
> Looks like this failed to build on Windows.

And Qt.  Their compilers don't agree on what default arguments mean.  I'm fixing.

> > WebKit2/Shared/SessionState.cpp:62
> > +    // This might change.
> 
> This comment is cryptic. I think what you mean to say is:
> 
>     // Because this might change later, callers should use this instead of calling list().isEmpty() directly themselves.

That's better.  Done.

> > WebKit2/UIProcess/WebPageProxy.cpp:212
> > +    reattachToWebProcess(true);
> 
> The boolean here is not good. It’s unclear what “true” means. I’d rather see an enum used to make call sites like this one understandable.

I'll do this.

> > WebKit2/UIProcess/WebPageProxy.cpp:238
> > -    process()->send(Messages::WebProcess::CreateWebPage(m_pageID, creationParameters()), 0);
> > +    process()->send(Messages::WebProcess::CreateWebPage(m_pageID, creationParameters(reloadCurrentSessionHistoryItem)), 0);
> 
> Instead of sending a boolean, could we just send a second message to trigger the reloading?

I'd thought about this carefully and decided not to pursue it.  There's already too much asynchronicity and too many messages in (re)creating a WebPage, and I didn't think it was worth it to go down that road.

If you feel strongly about this, I can revisit it, but it might take awhile.

> > WebKit2/UIProcess/WebPageProxy.cpp:377
> > +    if (!isValid()) {
> > +        reattachToWebProcessWithItem(m_backForwardList->forwardItem());
> >          return;
> > +    }
> 
> Might be over-engineering to support this. How did you test it?

Easy to make it possible in a test app with a couple of lines
> 
> > WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:74
> > +void WebBackForwardListProxy::setHighestItemIDFromUIProcess(uint64_t itemID)
> 
> Do we have a race condition here? What if an ID is created before receiving the new highest item?

That's fine - at the time the UIProcess passed this number over, it was the highest ID known.  The only new IDs the UIProcess might have created are for UIProcess items (even numbers).  The WebProcess is only interested in making sure the next *odd* ID it creates is at least this high.

> 
> > WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:84
> > +    uniqueHistoryItemID = itemID;
> > +
> > +    // If the highest used ID is even, increment it to the next odd ID as that is what should be used
> > +    // for items generated by the WebProcess. 
> > +    if (uniqueHistoryItemID % 2 == 0)
> > +        ++uniqueHistoryItemID;
> 
> I think this would read better if you set uniqueHistoryItemID only once. Maybe like this:
> 
>     if (itemID % 2)
>         uniqueHistoryItemID = itemID;
>     else
>         uniqueHistoryItemID = itemID + 1;
> 
> Or with a function:
> 
>     static uint64_t makeOdd(uint64_t number)
>     {
>         return number + number % 2;
>     }
> 
> And:
> 
>     uniqueHistoryItemID = makeOdd(itemID);

Will do.

[Brady Eidson]

Created attachment 78632 [details]
Patch v2 - Remove all those argument changes (all builds should be okay) and do it in 2 messages

[WebKit Review Bot]

Attachment 78632 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/Shared/Sessi..." exit_code: 1
WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:44:  The parameter name "itemID" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 78632 [details]
Patch v2 - Remove all those argument changes (all builds should be okay) and do it in 2 messages

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

> WebKit2/UIProcess/WebPageProxy.h:135
> +    void reattachToWebProcess();

Can this function be made private?

> WebKit2/WebProcess/WebPage/WebPage.cpp:885
> +#ifndef NDEBUG

This should probably be #ifndef ASSERT_DISABLED

> WebKit2/WebProcess/WebPage/WebPage.cpp:900
> +#ifndef NDEBUG

This one too.

[Brady Eidson]

Created attachment 78715 [details]
Patch v3 - Feedback from Sam here and in person

[WebKit Review Bot]

Attachment 78715 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/Shared/Sessi..." exit_code: 1

WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:44:  The parameter name "itemID" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

r75631