863140 - nsIDocument::GetInnerWindow sometimes returns an outer window

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Can we just fix that, please?  I would have thought bug 820170 would fix it, but it doesn't seem to have....

Comment 1

[Boris Zbarsky [:bzbarsky]]

And when we do, look at GetInnerWindow callers who are now checking to make sure they really got an inner.  :(

Comment 2

[Gabor Krizsanits (INACTIVE)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Gabor Krizsanits (INACTIVE)]

(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 :)

Comment 5

[Gabor Krizsanits (INACTIVE)]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

.defaultView returns the outer window, even on the c++ level... Why would that change cause its behavior to change?

Comment 7

[Gabor Krizsanits (INACTIVE)]

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?

Comment 8

[Gabor Krizsanits (INACTIVE)]

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)

Comment 9

[Gabor Krizsanits (INACTIVE)]

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.

Comment 10

[Gabor Krizsanits (INACTIVE)]

Attachment: nsIDocument::GetInnerWindow should never return the outer window

Comment 11

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: no need for those silly IsInnerWindow checks after GetInnerWindow any longer

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Gabor Krizsanits (INACTIVE)]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Gabor Krizsanits (INACTIVE)]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Gabor Krizsanits (INACTIVE)]

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

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

XHR document's don't have docshell/container.

Comment 20

[Gabor Krizsanits (INACTIVE)]

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

Comment 21

[Gabor Krizsanits (INACTIVE)]

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?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Gabor Krizsanits (INACTIVE)]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

That sounds plausible to me...

Comment 25

[Gabor Krizsanits (INACTIVE)]

Attachment: nsIDocument::GetInnerWindow should never return the outer window

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.

Comment 26

[Boris Zbarsky [: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.

Comment 27

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=f483ff30b73a

https://hg.mozilla.org/integration/mozilla-inbound/rev/33b755e0237d

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33b755e0237d