Bug 185958 – NavigationAction does not need to hold initiating DOM Event

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Simon Fraser mentioned in bug 185712, comment 4 that NavigationAction retains a DOM Event, which ultimately keeps the document associated with the DOM Event alive. It is sufficient for NavigationAction to own details of the initiating DOM Event as opposed to the DOM Event itself.

[Radar WebKit Bug Importer]

<rdar://problem/40531539>

[Daniel Bates]

Created attachment 341221 [details]
Patch

This patch depends on the patch for bug #185712.

[Daniel Bates]

Created attachment 341356 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 341356 [details]
Patch

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

> Source/WebCore/loader/NavigationAction.cpp:96
>  NavigationAction::NavigationAction(Document& requester, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, NavigationType type, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, Event* event, const AtomicString& downloadAttribute)

Maybe add a comment in the header file to say that this class shouldn't hold on to any objects from the originating document.

> Source/WebCore/loader/NavigationAction.h:93
> +        bool buttonDown;
> +        unsigned short button;
> +        unsigned short syntheticClickType;
> +        LayoutPoint absoluteLocation;
> +        FloatPoint locationInRootViewCoordinates;

This would pack better with the larger types first.

[Daniel Bates]

Comment on attachment 341356 [details]
Patch

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

>> Source/WebCore/loader/NavigationAction.cpp:96
>>  NavigationAction::NavigationAction(Document& requester, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, NavigationType type, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, Event* event, const AtomicString& downloadAttribute)
> 
> Maybe add a comment in the header file to say that this class shouldn't hold on to any objects from the originating document.

Will add the following comment above the class definition in the header before landing as formatted:

// NavigationAction should never hold a strong reference to the originating document either directly
// or indirectly as doing so prevents its destruction even after navigating away from it because
// DocumentLoader keeps around the NavigationAction for the last navigation.

Will also add the following comment below the "private" section in the header before landing as formatted:

// Do not add a strong reference to the originating document or a subobject that holds the
// originating document. See comment above the class for more details.

>> Source/WebCore/loader/NavigationAction.h:93
>> +        FloatPoint locationInRootViewCoordinates;
> 
> This would pack better with the larger types first.

Will order these fields as follows: locationInRootViewCoordinates, absoluteLocation, button, syntheticClickType, buttonDown.

[Daniel Bates]

Committed r232316: <https://trac.webkit.org/changeset/232316>