431767 - Investigate nsDocument::GetScriptGlobalObject after the document has been removed from its docshell

Status: RESOLVED DUPLICATE of bug 864335

(Core :: DOM: Core & HTML, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

The document's script global object is currently the inner window. After it has been closed, it starts using its docshell's script global object, which is an outer window. This causes confusion (e.g. from nsDocument::GetInnerWindow) when late calls to GSGO assume they are given back an inner window. We should figure out who calls GetScriptGlobalObject this late and what object they really want.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip

This patch removes GetScriptGlobalObject altogether. I still need to run mochitests and investigate an assertion I see when closing windows.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to trunk

I can no longer reproduce the assertion I mention in comment 1.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 362324 [details] [diff] [review]
Updated to trunk

- In content/base/public/nsIDocument.h:

   /**
    * Get/set the object from which a document can get a script context
    * and scope. This is the context within which all scripts (during
    * document creation and during event handling) will run. Note that
    * this is the *inner* window object.
+   *
+   * XXX Document me!
    */
-  virtual nsIScriptGlobalObject* GetScriptGlobalObject() const = 0;
   virtual void SetScriptGlobalObject(nsIScriptGlobalObject* aGlobalObject) = 0;

Yeah, document, and update the comment to not mention get any more.

   /**
    * Return the window containing the document (the outer window).
    */
-  virtual nsPIDOMWindow *GetWindow() = 0;
+  virtual nsPIDOMWindow *GetOuterWindow() = 0;

Maybe update this comment too?

- In nsXBLProtoImplAnonymousMethod::Execute():

-  nsIScriptGlobalObject* global = document->GetScriptGlobalObject();
+  nsCOMPtr<nsIScriptGlobalObject> global =
+      do_QueryInterface(document->GetInnerWindow());

This should probably use GetScopeObject().

r=jst with that. I'd love to have bz look this one over too.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fix the comment on GetInnerWindow() too to mention GetOuterWindow()?

And on GetScopeObject() to not mention "the script global object"?

In IsScriptEnabled, why not just use GetScopeObject() for the nsIScriptGlobalObject?  Why do the QI on GetOuterWindow?

I wonder whether we should have a QueryContainer() helper that would do_QueryReferent and return that instead of the current CallQueryReferent to nsISupports followed by QI in the caller...  In fact, if we did I think we could nuke GetContainer.  Want to file a followup bug on this?

And then we could also remove the docshell stuff in nsScriptLoader.  No need for that at all.

Do we have a bug (followup?) on the scriptloader XXX comments about IsScriptEnabled?  If we can remove code there, let us!

In nsBindingManager::GetBindingImplementation, should we not use GetScopeObject()?

In nsImageDocument, don't we want the outer window?  We're calling GetFrameElementInternal() on the window, after all.

Why not the inner window in XBLResolve?  Or even GetScopeObject()?  Similar in nsXBLBinding::AllowScripts.

For the XBLProtoImplAnonymousMethod::Execute, why not GetScopeObject?

Why not GetScopeObject() in nsXULContentSink?

Should nsDocumentSH::PostCreate really be comparing SameCOMIdentity on the extant document of the _outer_ window, not the inner?

In nsFocusController::UpdateCommands, why inner window, not outer?

nsFormFillController::GetDocShellForInput should probably QI the container.

The rest looks good.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 362324 [details] [diff] [review]
Updated to trunk

sr- pending response to my comments.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Merged to trunk

I'll go through bz's comments now.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #4)
> Fix the comment on GetInnerWindow() too to mention GetOuterWindow()?
> 
> And on GetScopeObject() to not mention "the script global object"?
> 
> In IsScriptEnabled, why not just use GetScopeObject() for the
> nsIScriptGlobalObject?  Why do the QI on GetOuterWindow?

The idea is that technically the context lives on the outer, but I'll switch it because GetContext forwards and to save the QI.

> Why not the inner window in XBLResolve?  Or even GetScopeObject()?  Similar in
> nsXBLBinding::AllowScripts.

Same reason as above, but I'll switch it.

> Should nsDocumentSH::PostCreate really be comparing SameCOMIdentity on the
> extant document of the _outer_ window, not the inner?

I don't think so. We're defining window.document on the *inner* window there, not the outer. I think that check exists only to prevent us defining an old document if someone did document.write or something.

New patch coming soon.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> We're defining window.document on the *inner* window there, not the outer. I
> think that check exists only to prevent us defining an old document if someone
> did document.write or something.

That doesn't make sense.  I can see us doing the check to deal with about:blank documents switching out from under an inner window, though...  Add a comment to that code?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Duping forward to the bug that fix this!