Bug 27640 – DOMWindow::document() should not reach through Frame

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric Seidel (no email)]

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());
        }

[Eric Seidel (no email)]

The ASSERT is hit when running fast/dom/gc-6.html, due creating a DOMObject after the document has been detached from the frame.

[Adam Barth]

*** Bug 93666 has been marked as a duplicate of this bug. ***

[Adam Barth]

Created attachment 158164 [details]
Works in JavaScriptCore

[Sam Weinig]

Comment on attachment 158164 [details]
Works in JavaScriptCore

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.

[Adam Barth]

Ok.  Will do.  Thanks for taking a look at the patch.

[Adam Barth]

Created attachment 158201 [details]
For EWS

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 158201 [details]
For EWS

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

[WebKit Review Bot]

Created attachment 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

[Adam Barth]

> fast/dom/xmlhttprequest-constructor-in-detached-document.html

This turns out to be a progression.  The warning message is gone.  :)

[Adam Barth]

Created attachment 158257 [details]
Patch

[Adam Barth]

Created attachment 158258 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 158257 [details]
Patch

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.

[Adam Barth]

(In reply to comment #13)
> (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. :(

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.

[Adam Barth]

Created attachment 158369 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 158369 [details]
Patch for landing

Clearing flags on attachment: 158369

Committed r125592: <http://trac.webkit.org/changeset/125592>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

(In reply to comment #17)
> All reviewed patches have been landed.  Closing bug.

Very nice!

[Darin Adler]

Comment on attachment 158369 [details]
Patch for landing

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?

[Adam Barth]

(In reply to comment #19)
> (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.

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.

[Csaba Osztrogonác]

(In reply to comment #16)
> (From update of attachment 158369 [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?

[Adam Barth]

> It caused a regression: https://bugs.webkit.org/show_bug.cgi?id=94119
> Could you check it, please?

Yes, thank you.

[Adam Barth]

(In reply to comment #20)
> (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.

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