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

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: khuey, Assigned: florian)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxperf])

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

a year ago
This code was introduced in bug 82368, 17 years ago for Mozilla 0.9.1.
Blocks: 82368
Assignee

Comment 2

a year ago
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).
Blocks: 1336227
Assignee

Comment 3

a year ago
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.
Attachment #8947510 - Flags: review?(bugs)
Assignee

Updated

a year ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Whiteboard: [fxperf]
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?
Attachment #8947510 - Flags: review?(bugs) → review-
Assignee

Comment 5

a year ago
Posted patch Patch v2Splinter Review
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.
Attachment #8948356 - Flags: review?(jvarga)
Assignee

Updated

a year ago
Attachment #8947510 - Attachment is obsolete: true
Please ask someone else, I'm currently busy with a project and not doing reviews for stuff like this. Thanks.
Assignee

Updated

a year ago
Attachment #8948356 - Flags: review?(jvarga) → review?(nika)
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?
Attachment #8948356 - Flags: review?(nika) → review+

Comment 8

a year ago
Pushed by florian@queze.net:
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.
https://hg.mozilla.org/mozilla-central/rev/31a6b24710a5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.