RESOLVED FIXED 93990
Delete Frame::domWindow() and Frame::existingDOMWindow()
https://bugs.webkit.org/show_bug.cgi?id=93990
Summary Delete Frame::domWindow() and Frame::existingDOMWindow()
WebKit Review Bot
Reported 2012-08-14 10:15:41 PDT
Delete Frame::domWindow() and Frame::existingDOMWindow() Requested by abarth on #webkit.
Attachments
Patch (85.70 KB, patch)
2012-08-14 13:45 PDT, Adam Barth
no flags
Patch (87.70 KB, patch)
2012-08-14 14:43 PDT, Adam Barth
eric: review+
Patch for landing (87.66 KB, patch)
2012-08-14 15:23 PDT, Adam Barth
no flags
For landing once EWS cycle green (87.63 KB, patch)
2012-08-14 15:24 PDT, Adam Barth
no flags
Adam Barth
Comment 1 2012-08-14 13:45:09 PDT
Build Bot
Comment 2 2012-08-14 14:02:09 PDT
Early Warning System Bot
Comment 3 2012-08-14 14:03:21 PDT
Eric Seidel (no email)
Comment 4 2012-08-14 14:08:45 PDT
Comment on attachment 158406 [details] Patch You borks it.
Adam Barth
Comment 5 2012-08-14 14:09:23 PDT
Yeah, I need to fight the EWS for a bit.
Adam Barth
Comment 6 2012-08-14 14:43:30 PDT
Eric Seidel (no email)
Comment 7 2012-08-14 14:55:49 PDT
Comment on attachment 158418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158418&action=review Looks reasonable. > Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:59 > + return v8::Undefined(); I think you mean v8Undefined. :) > Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:61 > + if (!document->frame()) > + return v8::Undefined(); // FIXME: How could Frame be null here? Seems like this would be possible from JS, no? Grab the content document of a frame, then disconnect the frameset from the document (removeChild it), and then try and grab a named property on teh frameset? > Source/WebCore/svg/SVGDocumentExtensions.cpp:199 > + if (document->frame()) do you wnat to check frame here? > Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:148 > + if (ownerDocument()->frame()) Do you want to check frame here?
Adam Barth
Comment 8 2012-08-14 15:02:04 PDT
Comment on attachment 158418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158418&action=review >> Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:59 >> + return v8::Undefined(); > > I think you mean v8Undefined. :) Shoot me now. >> Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:61 >> + return v8::Undefined(); // FIXME: How could Frame be null here? > > Seems like this would be possible from JS, no? Grab the content document of a frame, then disconnect the frameset from the document (removeChild it), and then try and grab a named property on teh frameset? Yes, you're right. >> Source/WebCore/svg/SVGDocumentExtensions.cpp:199 >> + if (document->frame()) > > do you wnat to check frame here? I think checking document->domWindow() for 0 is equivalent, but I wanted to make this change as mechanical as possible. >> Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:148 >> + if (ownerDocument()->frame()) > > Do you want to check frame here? ditto
Adam Barth
Comment 9 2012-08-14 15:23:01 PDT
Created attachment 158422 [details] Patch for landing
Adam Barth
Comment 10 2012-08-14 15:24:13 PDT
Created attachment 158423 [details] For landing once EWS cycle green
Adam Barth
Comment 11 2012-08-14 15:34:09 PDT
Darin Adler
Comment 12 2012-08-14 17:04:50 PDT
Comment on attachment 158423 [details] For landing once EWS cycle green View in context: https://bugs.webkit.org/attachment.cgi?id=158423&action=review > Source/WebCore/bindings/js/JSEventListener.cpp:-95 > - // The window must still be active in its frame. See <https://bugs.webkit.org/show_bug.cgi?id=21921>. > - // FIXME: A better fix for this may be to change DOMWindow::frame() to not return a frame the detached window used to be in. What made this comment obsolete? > Source/WebCore/loader/DocumentLoader.cpp:425 > - if (DOMWindow* window = m_frame->existingDOMWindow()) > + if (DOMWindow* window = m_frame->document()->domWindow()) Can domWindow ever return null? Why null check? > Source/WebCore/loader/FrameLoader.cpp:467 > - if (DOMWindow* window = m_frame->existingDOMWindow()) { > + if (DOMWindow* window = m_frame->document()->domWindow()) { Can domWindow ever return null? Why null check? > Source/WebCore/loader/FrameLoader.cpp:1905 > - if (DOMWindow* window = m_frame->existingDOMWindow()) { > + if (DOMWindow* window = m_frame->document()->domWindow()) { Can domWindow ever return null? Why null check? > Source/WebCore/loader/FrameLoader.cpp:2697 > - DOMWindow* domWindow = m_frame->existingDOMWindow(); > + DOMWindow* domWindow = m_frame->document()->domWindow(); Can domWindow ever return null? Why null check? > Source/WebCore/page/Frame.h:-133 > - DOMWindow* existingDOMWindow() { return domWindow(); } Did we lose an optimization at some point earlier in this work where we’d avoid creating a DOMWindow in some cases?
Adam Barth
Comment 13 2012-08-14 17:35:38 PDT
Comment on attachment 158423 [details] For landing once EWS cycle green View in context: https://bugs.webkit.org/attachment.cgi?id=158423&action=review >> Source/WebCore/bindings/js/JSEventListener.cpp:-95 >> - // FIXME: A better fix for this may be to change DOMWindow::frame() to not return a frame the detached window used to be in. > > What made this comment obsolete? This FIXME is redundant with the FIXME comment above DOMWindow::isCurrentlyDisplayedInFrame. Now that this code calls isCurrentlyDisplayedInFrame, I thought we didn't need the FIXME comment here. >> Source/WebCore/loader/DocumentLoader.cpp:425 >> + if (DOMWindow* window = m_frame->document()->domWindow()) > > Can domWindow ever return null? Why null check? Document::domWindow() can return 0 for document that were never active in a Frame (e.g., a Document created by XMLHttpRequest). In this instance, we're getting the Document from a Frame, so it's obviously active in that frame. I've filed Bug 94052 about removing these extra null checks. >> Source/WebCore/page/Frame.h:-133 >> - DOMWindow* existingDOMWindow() { return domWindow(); } > > Did we lose an optimization at some point earlier in this work where we’d avoid creating a DOMWindow in some cases? I don't think so. As far as I can tell, every active Document ended up with a DOMWindow. For example, FrameLoader::clear ends up creating one. It's possible that we could have avoided creating the DOMWindow before we had a split window. I'm not as familiar with how things worked then. DOMWindow is actually a pretty light object. It's just a bunch of 0 RefPtrs that get filled in lazily.
Note You need to log in before you can comment on or make changes to this bug.