1223303 - nsChromeTreeOwner::OnLocationChanged is very confused about what a docshell and a window are

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/annotate/cd345a81ac34/xpfe/appshell/nsChromeTreeOwner.cpp#l543

It QIs a docshell to an nsIDOMWindow, essentially guaranteeing that the comparison on line 552 always fails.  I'm not sure what to do with this, but filing so I have a bug # to reference in my comment.

Comment 1

[Florian Quèze [:florian]]

This code was introduced in bug 82368, 17 years ago for Mozilla 0.9.1.

Comment 2

[Florian Quèze [:florian]]

Fixing this bug is required for bug 1336227 (and more specifically to handle the chromemargin attribute when loading browser.xul in an existing about:blank window).

Comment 3

[Florian Quèze [:florian]]

Attachment: Fix nsChromeTreeOwner::OnLocationChanged to set nsXULWindow::mChromeLoaded to false when loading a different document,

I think this is the straight forward fix, but I haven't been able to find a case where isTopLevel is false in my local testing.

Comment 4

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

Comment on attachment 8947510 [details] [diff] [review]
Fix nsChromeTreeOwner::OnLocationChanged to set nsXULWindow::mChromeLoaded to false when loading a different document,

Conceptually this is wrong, I think.

Could you QI docshell to nsIWebProgress and then just compare that it is the same as 
aWebProgress?

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v2

New version using the suggestion from comment 4. This seems to work fine, and unless in the previous version, the test doesn't always returns the same value :-).
Changing reviewer as smaug is not accepting requests during his vacations.

Comment 6

[Jan Varga [:janv]]

Please ask someone else, I'm currently busy with a project and not doing reviews for stuff like this. Thanks.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8948356 [details] [diff] [review]
Patch v2

Review of attachment 8948356 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpfe/appshell/nsChromeTreeOwner.cpp
@@ +466,4 @@
>     return NS_OK;
>  }
>  
>  NS_IMETHODIMP nsChromeTreeOwner::OnLocationChange(nsIWebProgress* aWebProgress,

nit: while you're in here want to move the NS_IMETHODIMP one line earlier?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31a6b24710a5
Fix nsChromeTreeOwner::OnLocationChanged to set nsXULWindow::mChromeLoaded to false when loading a different document, r=mystor.

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/31a6b24710a5