Use std::optional<NavigationAction> to represent an empty NavigationAction

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Daniel Bates]

Use std::optional<NavigationAction> to represent the "empty NavigationAction" and require a NavigationAction to be instantiated with a ResourceRequest.

[Daniel Bates]

Created attachment 313745 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

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.

[Daniel Bates]

Comment on attachment 313745 [details]
Patch

Clearly, a load can be initiated with an empty URL.

[Daniel Bates]

(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.