Bug 27640

Summary: DOMWindow::document() should not reach through Frame
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: DOMAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, dglazkov, haraken, japhet, ossy, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: 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 Eric Seidel (no email) 2009-07-23 22:57:46 PDT
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 Eric Seidel (no email) 2009-07-23 23:00:25 PDT
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 Adam Barth 2012-08-13 17:35:25 PDT
*** Bug 93666 has been marked as a duplicate of this bug. ***
Comment 3 Adam Barth 2012-08-13 17:39:21 PDT
Created attachment 158164 [details]
Works in JavaScriptCore
Comment 4 Sam Weinig 2012-08-13 18:20:15 PDT
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.
Comment 5 Adam Barth 2012-08-13 18:52:42 PDT
Ok.  Will do.  Thanks for taking a look at the patch.
Comment 6 Adam Barth 2012-08-13 21:20:05 PDT
Created attachment 158201 [details]
For EWS
Comment 7 Adam Barth 2012-08-13 21:20:51 PDT
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 WebKit Review Bot 2012-08-13 22:39:29 PDT
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
Comment 9 WebKit Review Bot 2012-08-13 22:39:33 PDT
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
Comment 10 Adam Barth 2012-08-13 23:58:54 PDT
> fast/dom/xmlhttprequest-constructor-in-detached-document.html

This turns out to be a progression.  The warning message is gone.  :)
Comment 11 Adam Barth 2012-08-14 01:24:18 PDT
Created attachment 158257 [details]
Patch
Comment 12 Adam Barth 2012-08-14 01:27:32 PDT
Created attachment 158258 [details]
Patch
Comment 13 Eric Seidel (no email) 2012-08-14 02:01:02 PDT
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.
Comment 14 Adam Barth 2012-08-14 10:26:51 PDT
(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.
Comment 15 Adam Barth 2012-08-14 10:57:48 PDT
Created attachment 158369 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-08-14 12:48:16 PDT
Comment on attachment 158369 [details]
Patch for landing

Clearing flags on attachment: 158369

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

Very nice!
Comment 19 Darin Adler 2012-08-14 16:52:01 PDT
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?
Comment 20 Adam Barth 2012-08-14 17:16:05 PDT
(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.
Comment 21 Csaba Osztrogonác 2012-08-15 09:06:34 PDT
(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?
Comment 22 Adam Barth 2012-08-15 09:29:00 PDT
> It caused a regression: https://bugs.webkit.org/show_bug.cgi?id=94119
> Could you check it, please?

Yes, thank you.
Comment 23 Adam Barth 2012-08-16 14:42:24 PDT
(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.