Bug 27640

Summary: DOMWindow::document() should not reach through Frame
Product: WebKit Reporter: Eric Seidel <eric@webkit.org>
Component: HTML DOMAssignee: Adam Barth <abarth@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, ap@webkit.org, dglazkov@chromium.org, haraken@chromium.org, japhet@chromium.org, ossy@webkit.org, sam@webkit.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Bug Depends on: 94119    
Bug Blocks: 27634, 27662, 75793    
Attachments:
Description Flags
Works in JavaScriptCore
none
For EWS
none
Archive of layout-test-results from gce-cr-linux-02
none
Patch
none
Patch
none
Patch for landing none

Description From 2009-07-23 22:57:46 PST
DOMWindow::document() should not reach through Frame

Document* DOMWindow::document() const
{
    // FIXME: This function shouldn't need a frame to work.
    if (!m_frame)
        return 0;


That FIXME causes us to hit an ASSERT in DOMObjectWithGlobalPointer() after bug 27634.  For now I've commented out the ASSERT.

        DOMObjectWithGlobalPointer(PassRefPtr<JSC::Structure> structure,
JSDOMGlobalObject* globalObject)
            : DOMObject(structure)
            , m_globalObject(globalObject)
        {
            ASSERT(globalObject->scriptExecutionContext());
        }
------- Comment #1 From 2009-07-23 23:00:25 PST -------
The ASSERT is hit when running fast/dom/gc-6.html, due creating a DOMObject after the document has been detached from the frame.
------- Comment #2 From 2012-08-13 17:35:25 PST -------
*** Bug 93666 has been marked as a duplicate of this bug. ***
------- Comment #3 From 2012-08-13 17:39:21 PST -------
Created an attachment (id=158164) [details]
Works in JavaScriptCore
------- Comment #4 From 2012-08-13 18:20:15 PST -------
(From update of attachment 158164 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=158164&action=review

> Source/WebCore/dom/Document.h:1276
> +    RefPtr<DOMWindow> m_defaultView;

I don't think we should call this m_defaultView, I think that just serves to confuse people.  We should be up front and call it the DOMWindow that it is, and keep the name defaultView() for IDL only as relic of DOM standards past.
------- Comment #5 From 2012-08-13 18:52:42 PST -------
Ok.  Will do.  Thanks for taking a look at the patch.
------- Comment #6 From 2012-08-13 21:20:05 PST -------
Created an attachment (id=158201) [details]
For EWS
------- Comment #7 From 2012-08-13 21:20:51 PST -------
That patch doesn't have the m_defaultView -> m_domWindow rename in it yet.  I'd just like to see what the EWS says.  If things go well, I'll polish it up and write ChangeLogs.
------- Comment #8 From 2012-08-13 22:39:29 PST -------
(From update of attachment 158201 [details])
Attachment 158201 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13493446

New failing tests:
fast/dom/xmlhttprequest-constructor-in-detached-document.html
------- Comment #9 From 2012-08-13 22:39:33 PST -------
Created an attachment (id=158218) [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
------- Comment #10 From 2012-08-13 23:58:54 PST -------
> fast/dom/xmlhttprequest-constructor-in-detached-document.html

This turns out to be a progression.  The warning message is gone.  :)
------- Comment #11 From 2012-08-14 01:24:18 PST -------
Created an attachment (id=158257) [details]
Patch
------- Comment #12 From 2012-08-14 01:27:32 PST -------
Created an attachment (id=158258) [details]
Patch
------- Comment #13 From 2012-08-14 02:01:02 PST -------
(From update of attachment 158257 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=158257&action=review

Looks great!  Please file your 8 victory lap bugs while they're still fresh in your head.  It would be bad if we forgot them. :(

> Source/WebCore/bindings/js/ScriptController.h:124
> +    void clearWindowShell(DOMWindow* newDOMWindow, bool goingIntoPageCache);

If you update the patch, you might change this to updateWindowShell, or replaceWindowInShell or some-such.  Do we need to ASSERT(newDOMWindow)? or anything about the new dom window?

> Source/WebCore/dom/Document.cpp:615
> +    // FIXME: Should we clear the default view when we detach from the Frame?

you mean "m_domWindow".

> Source/WebCore/loader/DocumentWriter.cpp:134
> +    m_frame->loader()->clear(document.get(), !shouldReuseDefaultView, !shouldReuseDefaultView);

You might add a FIXME about this name?

> Source/WebCore/loader/FrameLoader.cpp:506
> +void FrameLoader::clear(Document* newDocument, bool clearWindowProperties, bool clearScriptObjects, bool clearFrameView)

Do we need to ASSERT things about this newDocument?  That it's not null?  That it's different from m_frame->document?  Obviously clear() needs to be ripped appart, but before we go there, do we need to ASSERT sanity in the cases we care about?

> Source/WebCore/page/DOMWindow.cpp:1312
> +SecurityOrigin* DOMWindow::securityOrigin() const
> +{
> +    return document() ? document()->securityOrigin() : 0;
>  }

Should we ASSERT(document()) instead?  0 seems like a bad answer here.

> Source/WebCore/page/DOMWindow.h:86
> +    class DOMWindow : public RefCounted<DOMWindow>, public EventTarget, public ContextDestructionObserver, public FrameDestructionObserver, public Supplementable<DOMWindow> {

This line should probably wrap at this point...

> Source/WebCore/page/DOMWindow.h:92
> +        // FIXME: Document this insanity.
> +        void didSecureTransitionTo(Document*);

That's not a very useful fixme.  Better would be something like:

// This is used for blah, but not all cases are well understood.  We need to do X, and y, an z.
------- Comment #14 From 2012-08-14 10:26:51 PST -------
(In reply to comment #13)
> (From update of attachment 158257 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158257&action=review
> 
> Looks great!  Please file your 8 victory lap bugs while they're still fresh in your head.  It would be bad if we forgot them. :(

https://bugs.webkit.org/show_bug.cgi?id=93987
https://bugs.webkit.org/show_bug.cgi?id=93989
https://bugs.webkit.org/show_bug.cgi?id=93990
https://bugs.webkit.org/show_bug.cgi?id=93991
https://bugs.webkit.org/show_bug.cgi?id=93993
https://bugs.webkit.org/show_bug.cgi?id=93995

> > Source/WebCore/bindings/js/ScriptController.h:124
> > +    void clearWindowShell(DOMWindow* newDOMWindow, bool goingIntoPageCache);
> 
> If you update the patch, you might change this to updateWindowShell, or replaceWindowInShell or some-such.  Do we need to ASSERT(newDOMWindow)? or anything about the new dom window?

Will do in https://bugs.webkit.org/show_bug.cgi?id=93987

> > Source/WebCore/dom/Document.cpp:615
> > +    // FIXME: Should we clear the default view when we detach from the Frame?
> 
> you mean "m_domWindow".

Fixed.

> > Source/WebCore/loader/DocumentWriter.cpp:134
> > +    m_frame->loader()->clear(document.get(), !shouldReuseDefaultView, !shouldReuseDefaultView);
> 
> You might add a FIXME about this name?

Done.

> > Source/WebCore/loader/FrameLoader.cpp:506
> > +void FrameLoader::clear(Document* newDocument, bool clearWindowProperties, bool clearScriptObjects, bool clearFrameView)
> 
> Do we need to ASSERT things about this newDocument?  That it's not null?  That it's different from m_frame->document?  Obviously clear() needs to be ripped appart, but before we go there, do we need to ASSERT sanity in the cases we care about?

I'd like to work on this issue when we tackle clear().  It's not called from too many places, so it shouldn't be too hard to work out in the future.

> > Source/WebCore/page/DOMWindow.cpp:1312
> > +SecurityOrigin* DOMWindow::securityOrigin() const
> > +{
> > +    return document() ? document()->securityOrigin() : 0;
> >  }
> 
> Should we ASSERT(document()) instead?  0 seems like a bad answer here.

I've filed https://bugs.webkit.org/show_bug.cgi?id=93991 about just deleting this function.

> > Source/WebCore/page/DOMWindow.h:86
> > +    class DOMWindow : public RefCounted<DOMWindow>, public EventTarget, public ContextDestructionObserver, public FrameDestructionObserver, public Supplementable<DOMWindow> {
> 
> This line should probably wrap at this point...

Done.

> > Source/WebCore/page/DOMWindow.h:92
> > +        // FIXME: Document this insanity.
> > +        void didSecureTransitionTo(Document*);
> 
> That's not a very useful fixme.  Better would be something like:
> 
> // This is used for blah, but not all cases are well understood.  We need to do X, and y, an z.

Done.
------- Comment #15 From 2012-08-14 10:57:48 PST -------
Created an attachment (id=158369) [details]
Patch for landing
------- Comment #16 From 2012-08-14 12:48:16 PST -------
(From update of attachment 158369 [details])
Clearing flags on attachment: 158369

Committed r125592: <http://trac.webkit.org/changeset/125592>
------- Comment #17 From 2012-08-14 12:48:21 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #18 From 2012-08-14 13:48:27 PST -------
(In reply to comment #17)
> All reviewed patches have been landed.  Closing bug.

Very nice!
------- Comment #19 From 2012-08-14 16:52:01 PST -------
(From update of attachment 158369 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=158369&action=review

> Source/WebCore/page/DOMWindow.cpp:456
>  ScriptExecutionContext* DOMWindow::scriptExecutionContext() const
>  {
> -    return document();
> +    return ContextDestructionObserver::scriptExecutionContext();
>  }

This function should be deleted. DOMWindow already inherits a perfectly good scriptExecutionContext function from ContextDestructionObserver. I don’t see why it would need to override it with a function that calls through to the base class.

> Source/WebCore/page/DOMWindow.h:90
> +    // FIXME: DOMWindow shouldn't subclass FrameDestructionObserver and instead should get to Frame via its Document.
> +    class DOMWindow : public RefCounted<DOMWindow>
> +                    , public EventTarget
> +                    , public ContextDestructionObserver
> +                    , public FrameDestructionObserver
> +                    , public Supplementable<DOMWindow> {

Lining up the colons and commas doesn’t seem like our conventional style.

Can any of this derivation be changed to use private inheritance?
------- Comment #20 From 2012-08-14 17:16:05 PST -------
(In reply to comment #19)
> (From update of attachment 158369 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158369&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:456
> >  ScriptExecutionContext* DOMWindow::scriptExecutionContext() const
> >  {
> > -    return document();
> > +    return ContextDestructionObserver::scriptExecutionContext();
> >  }
> 
> This function should be deleted. DOMWindow already inherits a perfectly good scriptExecutionContext function from ContextDestructionObserver. I don’t see why it would need to override it with a function that calls through to the base class.

Will do.

> > Source/WebCore/page/DOMWindow.h:90
> > +    // FIXME: DOMWindow shouldn't subclass FrameDestructionObserver and instead should get to Frame via its Document.
> > +    class DOMWindow : public RefCounted<DOMWindow>
> > +                    , public EventTarget
> > +                    , public ContextDestructionObserver
> > +                    , public FrameDestructionObserver
> > +                    , public Supplementable<DOMWindow> {
> 
> Lining up the colons and commas doesn’t seem like our conventional style.

I think I got confused with the style we use for constructors.

> Can any of this derivation be changed to use private inheritance?

I don't think so, but I could be wrong.  The plan is to remove the FrameDestructionObserver base class entirely in Bug 93995.
------- Comment #21 From 2012-08-15 09:06:34 PST -------
(In reply to comment #16)
> (From update of attachment 158369 [details] [details])
> Clearing flags on attachment: 158369
> 
> Committed r125592: <http://trac.webkit.org/changeset/125592>

It caused a regression: https://bugs.webkit.org/show_bug.cgi?id=94119
Could you check it, please?
------- Comment #22 From 2012-08-15 09:29:00 PST -------
> It caused a regression: https://bugs.webkit.org/show_bug.cgi?id=94119
> Could you check it, please?

Yes, thank you.
------- Comment #23 From 2012-08-16 14:42:24 PST -------
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 158369 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158369&action=review
> > 
> > > Source/WebCore/page/DOMWindow.cpp:456
> > >  ScriptExecutionContext* DOMWindow::scriptExecutionContext() const
> > >  {
> > > -    return document();
> > > +    return ContextDestructionObserver::scriptExecutionContext();
> > >  }
> > 
> > This function should be deleted. DOMWindow already inherits a perfectly good scriptExecutionContext function from ContextDestructionObserver. I don’t see why it would need to override it with a function that calls through to the base class.
> 
> Will do.

If I delete this function, the compiler complains because scriptExecutionContext() is pure virtual in EventTarget.h.