Bug 93990

Created attachment 158406 [details] Patch

Comment on attachment 158406 [details] Patch Attachment 158406 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13488778

Comment on attachment 158406 [details] Patch Attachment 158406 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13493789

Comment on attachment 158406 [details] Patch You borks it.

Yeah, I need to fight the EWS for a bit.

Created attachment 158418 [details] Patch

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?

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

Created attachment 158422 [details] Patch for landing

Created attachment 158423 [details] For landing once EWS cycle green

Committed r125615: <http://trac.webkit.org/changeset/125615>

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?

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.