Bug 196503 – Fix assertion in http/tests/adClickAttribution/store-ad-click-attribution.html

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Fix assertion in http/tests/adClickAttribution/store-ad-click-attribution.html

[Alex Christensen]

Created attachment 366513 [details]
Patch

[John Wilander]

Comment on attachment 366513 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:4112
> +        if (navigation) {

This could also be done in the initial if statement, like so:
if (frame->isMainFrame() && navigationID && (navigation = navigationState().navigation(navigationID))) {

We should consider the same check in WebPageProxy::didFailLoadForFrame(), WebPageProxy::didSameDocumentNavigationForFrame(), WebPageProxy::decidePolicyForNavigationAction(), WebPageProxy::didStartProvisionalLoadForFrameShared(), WebPageProxy::didFinishDocumentLoadForFrame(), and WebPageProxy::didFinishLoadForFrame().

[John Wilander]

(In reply to John Wilander from comment #2)
> Comment on attachment 366513 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366513&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:4112
> > +        if (navigation) {
> 
> This could also be done in the initial if statement, like so:
> if (frame->isMainFrame() && navigationID && (navigation =
> navigationState().navigation(navigationID))) {
> 
> We should consider the same check in WebPageProxy::didFailLoadForFrame(),
> WebPageProxy::didSameDocumentNavigationForFrame(),
> WebPageProxy::decidePolicyForNavigationAction(),
> WebPageProxy::didStartProvisionalLoadForFrameShared(),
> WebPageProxy::didFinishDocumentLoadForFrame(), and
> WebPageProxy::didFinishLoadForFrame().

I should say similar check. What I mean is, shouldn't we always check the returned navigation before it's used?

[Alex Christensen]

Created attachment 366523 [details]
Patch

[Alex Christensen]

(In reply to John Wilander from comment #3)
> I should say similar check. What I mean is, shouldn't we always check the
> returned navigation before it's used?
The navigation is not used in those other functions.

[John Wilander]

(In reply to Alex Christensen from comment #5)
> (In reply to John Wilander from comment #3)
> > I should say similar check. What I mean is, shouldn't we always check the
> > returned navigation before it's used?
> The navigation is not used in those other functions.

It's sent as parameter to other functions. Are you saying they (should) deal gracefully with null pointers?

In WebPageProxy::didFailLoadForFrame():

    if (m_loaderClient)
        m_loaderClient->didFailLoadWithErrorForFrame(*this, *frame, navigation.get(), error, m_process->transformHandlesToObjects(userData.object()).get());
    else if (frame->isMainFrame())
        m_navigationClient->didFailNavigationWithError(*this, *frame, navigation.get(), error, m_process->transformHandlesToObjects(userData.object()).get());


In WebPageProxy::didSameDocumentNavigationForFrame():

if (isMainFrame)
        m_navigationClient->didSameDocumentNavigation(*this, navigation.get(), navigationType, m_process->transformHandlesToObjects(userData.object()).get());


In WebPageProxy::didStartProvisionalLoadForFrameShared():

    if (m_loaderClient)
        m_loaderClient->didStartProvisionalLoadForFrame(*this, *frame, navigation.get(), process->transformHandlesToObjects(userData.object()).get());
    else if (frame->isMainFrame())
        m_navigationClient->didStartProvisionalNavigation(*this, navigation.get(), process->transformHandlesToObjects(userData.object()).get());

[Alex Christensen]

(In reply to John Wilander from comment #6)
> (In reply to Alex Christensen from comment #5)
> > (In reply to John Wilander from comment #3)
> > > I should say similar check. What I mean is, shouldn't we always check the
> > > returned navigation before it's used?
> > The navigation is not used in those other functions.
> 
> It's sent as parameter to other functions. Are you saying they (should) deal
> gracefully with null pointers?
They should.  Yes.  If they don't, then it's a different bug.

[John Wilander]

Comment on attachment 366523 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        during a cross-origin navigation, but if the old web process sends the message after WebPageProxy::commitProvisionalPage

Nit: I think it's cross-site, not cross-origin.

> Source/WebKit/UIProcess/WebPageProxy.cpp:-3878
> -        return;

Chris, did this code have some other purpose or is it OK to delete?

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:820
> +    if (auto* page = this->page())

Should be a newline here, right?

> Source/WebKit/WebProcess/WebPage/WebFrame.h:191
> +    bool m_navigationIsContinuingInAnotherProcess { false };

I like this name a lot more.

> LayoutTests/http/tests/adClickAttribution/store-ad-click-attribution.html:1
> +<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AdClickAttributionEnabled=true ] -->

No further change needed? The parser is OK with this and picks up both settings? If not, can wait until the other patch.

[Chris Dumez]

Comment on attachment 366523 [details]
Patch

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

>> Source/WebKit/ChangeLog:9
>> +        during a cross-origin navigation, but if the old web process sends the message after WebPageProxy::commitProvisionalPage
> 
> Nit: I think it's cross-site, not cross-origin.

Right.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:-3878
>> -        return;
> 
> Chris, did this code have some other purpose or is it OK to delete?

Sure, as long as we deal with the issue in the comment in another way, which Alex is doing in this patch via m_navigationIsContinuingInAnotherProcess.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4105
> +    if (frame->isMainFrame() && navigationID && (navigation = navigationState().navigation(navigationID))) {

This last check does not make much sense given that it would crash in debug. the navigation() getter has the following assertion:
ASSERT(m_navigations.contains(navigationID));

[John Wilander]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 366523 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366523&action=review
> 
> >> Source/WebKit/ChangeLog:9
> >> +        during a cross-origin navigation, but if the old web process sends the message after WebPageProxy::commitProvisionalPage
> > 
> > Nit: I think it's cross-site, not cross-origin.
> 
> Right.
> 
> >> Source/WebKit/UIProcess/WebPageProxy.cpp:-3878
> >> -        return;
> > 
> > Chris, did this code have some other purpose or is it OK to delete?
> 
> Sure, as long as we deal with the issue in the comment in another way, which
> Alex is doing in this patch via m_navigationIsContinuingInAnotherProcess.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:4105
> > +    if (frame->isMainFrame() && navigationID && (navigation = navigationState().navigation(navigationID))) {
> 
> This last check does not make much sense given that it would crash in debug.
> the navigation() getter has the following assertion:
> ASSERT(m_navigations.contains(navigationID));

I think what the patch is trying to encode is:
1) Still ASSERT on debug if the navigation is invalid. It shouldn't be so we want to find out through assert crashes.
2) Null check the navigation in release to avoid real crashes.

Alex, there was the GTK crash, right?

[EWS Watchlist]

Comment on attachment 366523 [details]
Patch

Attachment 366523 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11743219

New failing tests:
fast/shapes/shape-outside-floats/shape-outside-rounded-boxes-002.html
fast/frames/frame-deep-nested-resize.html
fast/shapes/shape-outside-floats/shape-outside-polygon-zero-vertex.html
fast/shapes/shape-outside-floats/shape-outside-one-pixel.html
fast/shapes/shape-outside-floats/shape-outside-shape-boxes-003.html
fast/shapes/shape-outside-floats/shape-outside-shape-boxes-002.html
fast/frames/frame-dead-region.html
fast/shapes/shape-outside-floats/shape-outside-rounded-boxes-001.html
fast/shapes/shape-outside-floats/shape-outside-shape-boxes-001.html
fast/shapes/shape-outside-floats/shape-outside-rounded-inset.html
fast/shapes/shape-outside-floats/shape-outside-relative-size-svg.html

[EWS Watchlist]

Created attachment 366539 [details]
Archive of layout-test-results from ews549 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews549  Port: win-future  Platform: CYGWIN_NT-10.0-2.11.1-0.329-5-3-x86_64-64bit

[Alex Christensen]

I think the patch is exactly the way it should be.  There is an assertion in the getter to make sure we catch any cases where we try to get a navigation that does not exist, but in release there is still a check so we don't crash if something does go wrong.  I can address the nit in the changelog when committing.

[Alex Christensen]

http://trac.webkit.org/r243767

[Radar WebKit Bug Importer]

<rdar://problem/49539020>

[Chris Dumez]

Comment on attachment 366523 [details]
Patch

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

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

This flag is never reset :(