Closed Bug 431767 Opened 15 years ago Closed 10 years ago

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

Categories

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

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 864335

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Flags: wanted-next+
Blocks: 432591
Attached patch wip (obsolete) — Splinter Review
This patch removes GetScriptGlobalObject altogether. I still need to run mochitests and investigate an assertion I see when closing windows.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attached patch Updated to trunk (obsolete) — Splinter Review
I can no longer reproduce the assertion I mention in comment 1.
Attachment #354220 - Attachment is obsolete: true
Attachment #362324 - Flags: superreview?(jst)
Attachment #362324 - Flags: review?(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.
Attachment #362324 - Flags: superreview?(jst)
Attachment #362324 - Flags: superreview?(bzbarsky)
Attachment #362324 - Flags: review?(jst)
Attachment #362324 - Flags: review+
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 on attachment 362324 [details] [diff] [review]
Updated to trunk

sr- pending response to my comments.
Attachment #362324 - Flags: superreview?(bzbarsky) → superreview-
Attached patch Merged to trunkSplinter Review
I'll go through bz's comments now.
Attachment #362324 - Attachment is obsolete: true
(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.
> 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?
Duping forward to the bug that fix this!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.