Use std::optional<NavigationAction> to represent the "empty NavigationAction" and require a NavigationAction to be instantiated with a ResourceRequest.
Created attachment 313745 [details] Patch
Comment on attachment 313745 [details] Patch Attachment 313745 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3986136 Number of test failures exceeded the failure limit.
Created attachment 313748 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 313745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313745&action=review This seems like a minor, subtle improvement but the patch seems kind of risky to me. > Source/WebCore/loader/NavigationAction.cpp:74 > + ASSERT(!m_resourceRequest.url().isEmpty()); Seems really tricky to be sure this is true! There are a lot of code paths we have to look at. My guess is that this assertion is the thing we are hitting on the mac-debug bot. > Source/WebCore/loader/PolicyCallback.cpp:80 > + ASSERT(m_navigationAction); I believe that the operator*/value() in our version of std::optional already does this assertion. So it seems excessive to also assert this when we are about to dereference a std::optional. In fact, this is one of the advantages that std::optional has over pointers. I’d suggest not adding this assertion. On the other hand, if we start using the std::optional from the standard libraries instead of our own copy from WTF, we might lose this behavior! > Source/WebCore/loader/PolicyCallback.cpp:81 > + m_newWindowFunction(m_request, m_formState.get(), m_frameName, *m_navigationAction, shouldContinue); I’m a little surprised that you are using * rather than the value() function. Not sure which we’d prefer or why but I think I have been mostly using value(). > Source/WebCore/loader/PolicyCallback.cpp:107 > + ASSERT(m_navigationAction); Ditto. > Source/WebCore/loader/PolicyCallback.cpp:108 > + m_newWindowFunction(m_request, m_formState.get(), m_frameName, *m_navigationAction, false); Ditto. > Source/WebCore/loader/PolicyChecker.cpp:88 > + auto& action = *loader->triggeringAction(); I’m concerned that someone might call setTriggeringAction later and invalidate this pointer. One way you could allay my concern would be to change the behavior of the setTriggeringAction function to do nothing if the loader already has a triggering action. Maybe even assert as well. But down below where action is used, the old code didn’t even count on accessing loader, so there could well be some unusual case where the document loader has already been destroyed by then. I don’t understand how we have confidence about the object lifetimes. > Source/WebCore/page/PerformanceNavigation.cpp:60 > + WebCore::NavigationType navigationType = triggeringAction->type(); I suggest using auto here instead of writing out the type.
Comment on attachment 313745 [details] Patch Clearly, a load can be initiated with an empty URL.
(In reply to Darin Adler from comment #4) > Comment on attachment 313745 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313745&action=review > > This seems like a minor, subtle improvement but the patch seems kind of > risky to me. > > > Source/WebCore/loader/NavigationAction.cpp:74 > > + ASSERT(!m_resourceRequest.url().isEmpty()); > > Seems really tricky to be sure this is true! There are a lot of code paths > we have to look at. > > My guess is that this assertion is the thing we are hitting on the mac-debug > bot. It is the assertion we are hitting on the mac-debug bot. I am not going to pursue this further unless you feel we should. Marking Resolved Invalid.