Closed Bug 863140 Opened 7 years ago Closed 7 years ago

nsIDocument::GetInnerWindow sometimes returns an outer window

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bzbarsky, Assigned: gkrizsanits)

References

Details

Attachments

(1 file, 2 obsolete files)

Can we just fix that, please?  I would have thought bug 820170 would fix it, but it doesn't seem to have....
And when we do, look at GetInnerWindow callers who are now checking to make sure they really got an inner.  :(
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Really? I was not aware of that. In that bug I wanted to preserve the current behavior of the document as much as possible. Anyway, is there a way to reproduce it? Or something we know about the typical scenario where it happens? I would guess that in GetScriptGlobalObject when we get the global from the mDocumentContainer, is the main culprit, so I would start there, but I can be totally wrong.
> I would guess that in GetScriptGlobalObject when we get the global from the
> mDocumentContainer

Yup, exactly.  mDocumentContainer is the docshell, so if we're going to there to get something we're getting an outer window.
(In reply to Boris Zbarsky (:bz) from comment #1)
> And when we do, look at GetInnerWindow callers who are now checking to make
> sure they really got an inner.  :(
Uhh... this looks really terrible.

I wanted to do some more cleaning up around the scope objects on nsDocument as a followup, I guess I should start here. mScopeObject is not nulled out like mScriptGlobalObject, so if the inner is still alive I can use that here instead of the docshell (that outer it returns might have a different inner already). Let's see how does try like this approach.

Oh, and if anyone feels an urge to fix this instead of me feel free to push me aside :)
Assignee: nobody → gkrizsanits
https://tbpl.mozilla.org/?tree=Try&rev=18b152fa416e

So there is one test that is failing: browser_inspector_destroyselection.js

In LayoutHelper.jsm there is a check if a node is connected:

let connected = (aNode.ownerDocument && aNode.ownerDocument.defaultView &&
                       !(aNode.compareDocumentPosition(aNode.ownerDocument.documentElement) &
                       aNode.DOCUMENT_POSITION_DISCONNECTED));

I don't think my change had any effect on the parent chain which compareDocumentPosition alg. is based on... So I suspected the defaultView was null previously here and after my change is not null, which turned out to be the case.

I could fix this by changing GetWindowInternal to still do what it did before, and get the outer window from the docshell, and then this test will likely pass again. I'm just not sure weather if that's the right thing to do. When should GetWindowInternal or GetDefaultView return null? Since this is the only thing that brakes I would expect this test should be changed, but I don't want to risk breaking the web... so I'm leaning toward keep using the docshell based solution here, and mScopeObject for the GetInnerWindow and GetScriptGlobalObject.
.defaultView returns the outer window, even on the c++ level... Why would that change cause its behavior to change?
Both .defaultView and GetInnerWindow calls GetScriptGlobalObject. GetScriptGlobalObject when the mScriptGlobalObject is already nulled got the window from the docshell (and returned it for both as the inner and as the outer...). Now that I changed GetScriptGlobalObject to use mScopeObject instead of the docshell, it seems to return the outer, even after the docshell approach would return null... Does that make sense?
Actually it's not clear to me why does the defaultView try to get the outer window once mWindow is null. I give it a try to just return mWindow for the defaultView and if that does not work out I will just preserve the old behavior for defaultView (getting the outer from the docshell when mWindow is null)
Blocks: 431767
this seemed to be the most reasonable version and it seems quite green:
https://tbpl.mozilla.org/?tree=Try&rev=286992217353

patch is coming soon.
> Both .defaultView and GetInnerWindow calls GetScriptGlobalObject. 

So...  Changing the behavior of GetScriptGlobalObject is really scary.  It's used in all sorts of code that predates the inner/outer window split and expects to just get a "something".  In particular, I think that making it return null or not based on GC (which is what this patch is doing) is pretty suboptimal.

I would be much much happier with just changing GetInnerWindowInternal to never call GetScriptGlobalObject and do sane things instead, and then we can worry about migrating consumers off GetScriptGlobalObject carefully, which we've been doing anyway.
One other note:  At least some of these callers might want to replace the IsInnerWindow() checks with checks that the inner is the current inner or something...
(In reply to Boris Zbarsky (:bz) from comment #12)
> > Both .defaultView and GetInnerWindow calls GetScriptGlobalObject. 
> 
> So...  Changing the behavior of GetScriptGlobalObject is really scary.  It's
> used in all sorts of code that predates the inner/outer window split and
> expects to just get a "something".  In particular, I think that making it
> return null or not based on GC (which is what this patch is doing) is pretty
> suboptimal.

I can do that... I just found this GetScriptGlobalObject returns the outer in some cases extremely scary and confusing to be honest. So I wanted to see what relies on it and it turns out that not much. It's also hard for me to understand why would anyone need the ScriptGlobalObject of a document when its (inner) window is dead. But I'll just do the safe thing here then and will try to dig into this a bit further in another bug.

> 
> I would be much much happier with just changing GetInnerWindowInternal to
> never call GetScriptGlobalObject and do sane things instead, and then we can
> worry about migrating consumers off GetScriptGlobalObject carefully, which
> we've been doing anyway.

Do you consider using mScopeObject in GetInnerWindowInternal a sane thing?

(In reply to Boris Zbarsky (:bz) from comment #13)
> One other note:  At least some of these callers might want to replace the
> IsInnerWindow() checks with checks that the inner is the current inner or
> something...

Thanks, this is a very good point I missed that...
> I just found this GetScriptGlobalObject returns the outer in some cases extremely scary
> and confusing to be honest

No argument there!

> It's also hard for me to understand why would anyone need the ScriptGlobalObject of a
> document when its (inner) window is dead

Some of the old consumers actually want the outer window, and rely on the auto-forwarding to the right window that windows do...  :(

> Do you consider using mScopeObject in GetInnerWindowInternal a sane thing?

Not sure.  Isn't mScopeObject set for things like XHR result documents and documents created with createDocument?
(In reply to Boris Zbarsky (:bz) from comment #15)
> Some of the old consumers actually want the outer window, and rely on the
> auto-forwarding to the right window that windows do...  :(

I really wonder why no tests were broken. Is there any better method to find these old consumers than checking all consumers and thinking over what they might assume?

> Not sure.  Isn't mScopeObject set for things like XHR result documents and
> documents created with createDocument?

Yes mScopeObject is always set. But weather it QI's to a window or not depends on if the owner global is a window or something else. For XHR result document the owner of the XHR will be it, for the ones created with createDocument it is very similar. But we do have tons of flags on nsDocument to fine tune it really. The question is what is the wanted behavior in these cases and if it's a concern that mScopeObject relies on the GC.
> I really wonder why no tests were broken.

Well, either because I'm wrong or because our test coverage is incomplete or both.  ;)

> Is there any better method to find these old consumers than checking all consumers and
> thinking over what they might assume?

I don't know of one.  :(

> Yes mScopeObject is always set.

Right, but does GetInnerWindow normally return non-null for XHR documents if the XHR came from a window?
Attachment #739583 - Flags: review?(bzbarsky) → review-
Attachment #739588 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #17)
> Right, but does GetInnerWindow normally return non-null for XHR documents if
> the XHR came from a window?

So here are the cases.
- mWindow is set, then nothing changes.
- !mRemovedFromDocShell, we use mScriptGlobalObject, nothing changes
- mRemovedFromDocShell, this is the interesting part. An I have no clue what the docshell would return in this case for XHR documents for example. If it returns something we return an outer (bug), if it does not I don't know if we consider it as a big or not... So what should we do? We can check if mWindow or mScriptGloblObject has been ever checked and return the inner from mScopeObject for example... Or we can check if mScopeObject is the current inner of the outer we get from the docshell and if so return it... But I don't know what is the right thing to do or how could I figure it out, because this part has never worked correctly.
XHR document's don't have docshell/container.
(In reply to Olli Pettay [:smaug] from comment #19)
> XHR document's don't have docshell/container.

In that case I think the get what doschell gives, check its current inner against mScopeObject, and if it's the same we can return it approach should work.
It would be nice though if I could just use mScopeObject directly after some checks (like if there is any container) without communicating to the docshell, but I don't have a clear overview here about all the cases for all the different documents. What would you guys prefer?
Ah, so I guess doing whatever we want in GetInnerWindowInternal is ok to some extent because mRemovedFromDocshell is never true for XHR result documents and the like, so those always return mWindow...

So ignore my worries about data documents.
(In reply to Boris Zbarsky (:bz) from comment #22)
> Ah, so I guess doing whatever we want in GetInnerWindowInternal is ok to
> some extent

I did quite some reading up on this area to get a better picture, and then checked all the current consumers of nsIDocument::GetInnerWindow. And currently I think the best first step would be just kill GetInnerWindowInternal and return null when mRemovedFromDocShell is set to true.

For consumers that checks if the inner we return is current inner this makes sense. They are not interesting right now in general.

The ones that checks if we return an outer we would just preserve the current behavior. We might need a current inner check there, but I don't think we really do.
- nsScriptLoader::EvaluateScript : this should only run when it's the current inner anyway, but we could check...
- various event handler getters and setters of various elements (GetOn##name etc.) : I'm not sure that allowing someone setting up an even listener on a bf cached document is a good idea, but disallowing it all of a sudden might break things.

And finally the consumers who does not check anything about the window we return just use it. I think in every case here it is a bug that it does not check for outer.
- nsContentUtils::HasMutationListeners - this should check for outer, so it's the previous category, current inner check should not be needed imo
- nsInProcessTabChildGlobal::PreHandleEvent - feels like the previous one, but really need confirmation here
- nsNodeUtils::CloneAndAdopt - this should check for outer too, don't think current inner would be needed
- nsEventListenerManager::GetInnerWindowForTarget - outer here seems to be wrong too
- TabParent::RecvPIndexedDBConstructor - this can now create an IDBFactory with the wrong inner window, so I think this is a bug. And since it's a constructor it should check for current inner, right?
- nsDocument::CanSavePresentation - I need help if I need an current inner check here or not
- nsPointerLockPermissionRequest::GetWindow - I have no idea...
- nsDocument::ResetToURI - shouldn't need current inner check
- any case that works only with the pres shells document should be fine I assume

What do you think? (anyone with deeper understanding than I have here... which is still not very deep...)
That sounds plausible to me...
I'm not sure if I have to change the iid for removing a protected method but I wanted to be on the safe side.
Attachment #739583 - Attachment is obsolete: true
Attachment #739588 - Attachment is obsolete: true
Attachment #741814 - Flags: review?(bzbarsky)
Comment on attachment 741814 [details] [diff] [review]
nsIDocument::GetInnerWindow should never return the outer window

Most of this is scary but fine.

>@@ -1127,16 +1127,22 @@ TabParent::RecvPIndexedDBConstructor(PIn

This change looks wrong to me.  Returning false from this method means "things are bad; kill the child process".  What you presumably want to do instead is to set *aAllowed to false and return true.

r=me with that.
Attachment #741814 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/33b755e0237d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 703831
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.