Bug 93990

Summary: Delete Frame::domWindow() and Frame::existingDOMWindow()
Product: WebKit Reporter: WebKit Review Bot <webkit.review.bot>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, gns, gyuyoung.kim, haraken, japhet, mifenton, philn, rakuco, tkent, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 75793    
Attachments:
Description Flags
Patch
none
Patch
eric: review+
Patch for landing
none
For landing once EWS cycle green none

Description WebKit Review Bot 2012-08-14 10:15:41 PDT
Delete Frame::domWindow() and Frame::existingDOMWindow()
Requested by abarth on #webkit.
Comment 1 Adam Barth 2012-08-14 13:45:09 PDT
Created attachment 158406 [details]
Patch
Comment 2 Build Bot 2012-08-14 14:02:09 PDT
Comment on attachment 158406 [details]
Patch

Attachment 158406 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13488778
Comment 3 Early Warning System Bot 2012-08-14 14:03:21 PDT
Comment on attachment 158406 [details]
Patch

Attachment 158406 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13493789
Comment 4 Eric Seidel (no email) 2012-08-14 14:08:45 PDT
Comment on attachment 158406 [details]
Patch

You borks it.
Comment 5 Adam Barth 2012-08-14 14:09:23 PDT
Yeah, I need to fight the EWS for a bit.
Comment 6 Adam Barth 2012-08-14 14:43:30 PDT
Created attachment 158418 [details]
Patch
Comment 7 Eric Seidel (no email) 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?
Comment 8 Adam Barth 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
Comment 9 Adam Barth 2012-08-14 15:23:01 PDT
Created attachment 158422 [details]
Patch for landing
Comment 10 Adam Barth 2012-08-14 15:24:13 PDT
Created attachment 158423 [details]
For landing once EWS cycle green
Comment 11 Adam Barth 2012-08-14 15:34:09 PDT
Committed r125615: <http://trac.webkit.org/changeset/125615>
Comment 12 Darin Adler 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?
Comment 13 Adam Barth 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.