Resurrect and fix layout test http/tests/adClickAttribution/store-ad-click-attribution.html

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Resurrect and fix layout test http/tests/adClickAttribution/store-ad-click-attribution.html

[Alex Christensen]

Created attachment 366459 [details]
Patch

[EWS Watchlist]

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


ERROR: LayoutTests/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 6 files


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

[Chris Dumez]

Comment on attachment 366459 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:282
> +        m_ignoreDestroyNavigationMessage = true;

In WebPageProxy::didDestroyNavigation(uint64_t navigationID), I had added the following code to address this issue:
    // On process-swap, the previous process tries to destroy the navigation but the provisional process is actually taking over the navigation.
    if (m_provisionalPage && m_provisionalPage->navigationID() == navigationID)
        return;

Any idea why this did not suffice?

[Alex Christensen]

At that point m_provisionalPage is null because it has been set to null by WebPageProxy::commitProvisionalPage.  I think it might be a race condition between the old page finishing stopping all loads and the new page committing the provisional load.  This test reliably hits the race condition so that it asserts.  This is a bug in our PSON code.

[Chris Dumez]

Comment on attachment 366459 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:282
>> +        m_ignoreDestroyNavigationMessage = true;
> 
> In WebPageProxy::didDestroyNavigation(uint64_t navigationID), I had added the following code to address this issue:
>     // On process-swap, the previous process tries to destroy the navigation but the provisional process is actually taking over the navigation.
>     if (m_provisionalPage && m_provisionalPage->navigationID() == navigationID)
>         return;
> 
> Any idea why this did not suffice?

We should probably drop my UI-side code at the same time then if your way is better / more robust.

[Alex Christensen]

I put the part of this that is specific to the PSON bug in https://bugs.webkit.org/show_bug.cgi?id=196503 and I'll add the code back to WebPageProxy::didCommitLoadForFrame and update the test expectations in a separate patch after that has landed to give that the possibility of being merged onto a branch without including pieces of unused features.

[Alex Christensen]

Created attachment 366546 [details]
Patch

[John Wilander]

Comment on attachment 366546 [details]
Patch

LGTM.

[John Wilander]

Chris, looks good to you too?

[Chris Dumez]

Comment on attachment 366546 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 366546 [details]
Patch

Clearing flags on attachment: 366546

Committed r243809: <https://trac.webkit.org/changeset/243809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49562994>