Bug 27640

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

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

Created attachment 158164 [details] Works in JavaScriptCore

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.

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

Created attachment 158201 [details] For EWS

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

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

> fast/dom/xmlhttprequest-constructor-in-detached-document.html This turns out to be a progression. The warning message is gone. :)

Created attachment 158257 [details] Patch

Created attachment 158258 [details] Patch

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.

(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.

Created attachment 158369 [details] Patch for landing

Comment on attachment 158369 [details] Patch for landing Clearing flags on attachment: 158369 Committed r125592: <http://trac.webkit.org/changeset/125592>

All reviewed patches have been landed. Closing bug.

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

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?

(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.

(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?

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

(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.