Fix assertion in http/tests/adClickAttribution/store-ad-click-attribution.html
Created attachment 366513 [details] Patch
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().
(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?
Created attachment 366523 [details] Patch
(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.
(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());
(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.
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.
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));
(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?
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
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
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.
http://trac.webkit.org/r243767
<rdar://problem/49539020>
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 :(