Bug 185958 - NavigationAction does not need to hold initiating DOM Event
Summary: NavigationAction does not need to hold initiating DOM Event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-24 13:06 PDT by Daniel Bates
Modified: 2018-05-30 16:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (20.82 KB, patch)
2018-05-24 13:22 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (20.82 KB, patch)
2018-05-25 16:52 PDT, Daniel Bates
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-05-24 13:06:48 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-05-24 13:07:24 PDT
<rdar://problem/40531539>
Comment 2 Daniel Bates 2018-05-24 13:22:29 PDT
Created attachment 341221 [details]
Patch

This patch depends on the patch for bug #185712.
Comment 3 Daniel Bates 2018-05-25 16:52:14 PDT
Created attachment 341356 [details]
Patch
Comment 4 Simon Fraser (smfr) 2018-05-25 17:08:39 PDT
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.
Comment 5 Daniel Bates 2018-05-30 16:23:50 PDT
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.
Comment 6 Daniel Bates 2018-05-30 16:30:21 PDT
Committed r232316: <https://trac.webkit.org/changeset/232316>