RESOLVED FIXED 185958
NavigationAction does not need to hold initiating DOM Event
https://bugs.webkit.org/show_bug.cgi?id=185958
Summary NavigationAction does not need to hold initiating DOM Event
Daniel Bates
Reported 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.
Attachments
Patch (20.82 KB, patch)
2018-05-24 13:22 PDT, Daniel Bates
no flags
Patch (20.82 KB, patch)
2018-05-25 16:52 PDT, Daniel Bates
simon.fraser: review+
Radar WebKit Bug Importer
Comment 1 2018-05-24 13:07:24 PDT
Daniel Bates
Comment 2 2018-05-24 13:22:29 PDT
Created attachment 341221 [details] Patch This patch depends on the patch for bug #185712.
Daniel Bates
Comment 3 2018-05-25 16:52:14 PDT
Simon Fraser (smfr)
Comment 4 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.
Daniel Bates
Comment 5 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.
Daniel Bates
Comment 6 2018-05-30 16:30:21 PDT
Note You need to log in before you can comment on or make changes to this bug.