[FIX] "ASSERTION: We should never be reusing a shared inner window" with pagehide event

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jruderman, Assigned: smaug)

Tracking

(Blocks 2 bugs, {assertion, sec-high, testcase})

Trunk
mozilla21
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 wontfix, firefox18- wontfix, firefox19+ verified, firefox20+ verified, firefox21+ verified, firefox-esr10- wontfix, firefox-esr1719+ verified, b2g1819+ fixed)

Details

(Whiteboard: [adv-main19+][adv-esr1703+])

Attachments

(3 attachments)

Posted file testcase
###!!! ASSERTION: We should never be reusing a shared inner window: '!currentInner->IsFrozen()', file dom/base/nsGlobalWindow.cpp, line 1776
Posted file stack trace
I'll look at this.
Assignee: nobody → bugs
guessing sec-high?
Keywords: sec-high
...still on my todo list.
Prevent loading new stuff to sub-docshell of unloaded ancestor.
This is pretty strict, but simple.

Running still some more tests, but at least docshell and shistory tests do pass.
Attachment #690353 - Flags: review?(bzbarsky)
Comment on attachment 690353 [details] [diff] [review]
simple approach

r=me
Attachment #690353 - Flags: review?(bzbarsky) → review+
Comment on attachment 690353 [details] [diff] [review]
simple approach

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Exact issue perhaps not so easily, but it is easy to see what case is being fixed.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The commit message will be about being more strict when we allow starting new loads

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply easily to all the branches

How likely is this patch to cause regressions; how much testing does it need?
This is somewhat regression risky. Needs more testing than patches to security bugs usually.
Attachment #690353 - Flags: sec-approval?
Summary: "ASSERTION: We should never be reusing a shared inner window" with pagehide event → [FIX] "ASSERTION: We should never be reusing a shared inner window" with pagehide event
Attachment #690353 - Flags: sec-approval? → sec-approval+
Can we get branch patches made or else have a discussion about whether or not this will be too risky to take in ESR17 or Firefox 18 or 19?
The same patch applies to FF18, FF19 and ESR17. (ESR17 with some fuzz).
Comment on attachment 690353 [details] [diff] [review]
simple approach

"This is somewhat regression risky. Needs more testing than patches to security bugs usually." means we shouldn't automatically sec-approval+, since we likely don't want this patch for FF18. This needs to be reconsidered.
Attachment #690353 - Flags: sec-approval+ → sec-approval?
What I meant with that comment is that this needs more real world testing, so I'm hoping to have this
in m-c for some time before landing elsewhere.
I read his statements Alex, when do you propose a better time would be to get it in. Even if we don't take it for 18, we still need to take it at some point. It is sec-high and a small patch, even if tricky.
(In reply to Al Billings [:abillings] from comment #12)
> I read his statements Alex, when do you propose a better time would be to
> get it in. Even if we don't take it for 18, we still need to take it at some
> point. It is sec-high and a small patch, even if tricky.

I'd say landing to all branches at the beginning of the FF19 cycle at this point, given risk. Or possibly using a cover bug to land on Central/Aurora sooner. But we don't want to take this fix for FF18 at this point (beta 5).
If Olli thinks it's risky then let's target Fx19 rather than 18. If it really needs testing then let's get it into 19-beta1, meaning land on Aurora prior to the merge (first week in January probably good).
Given that we're not landing this on 18 which is the final ESR10 build, wontfixing.
(In reply to Alex Keybl [:akeybl] from comment #13)
> (In reply to Al Billings [:abillings] from comment #12)
> > I read his statements Alex, when do you propose a better time would be to
> > get it in. Even if we don't take it for 18, we still need to take it at some
> > point. It is sec-high and a small patch, even if tricky.
> 
> I'd say landing to all branches at the beginning of the FF19 cycle at this
> point, given risk. Or possibly using a cover bug to land on Central/Aurora
> sooner. But we don't want to take this fix for FF18 at this point (beta 5).

So after January 8?
We're now past the merge, and ready to take a fix on all branches.
This needs approval
Comment on attachment 690353 [details] [diff] [review]
simple approach

Al set this previously, so setting to sec‑approval+ again.
Attachment #690353 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/224712fc024e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Olli Pettay [:smaug] from comment #20)
> https://hg.mozilla.org/mozilla-central/rev/224712fc024e

Please prepare patches for landing to all branches.
Comment on attachment 690353 [details] [diff] [review]
simple approach

based on my comment 9 this should apply to all the branches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old stuff
User impact if declined: Odd behavior, possible crashes
Testing completed: landed m-c 2013-01-08
Risk to taking this patch (and alternatives if risky): 
This is tiny bit risky. See comment 7 and comment 11
String or UUID changes made by this patch: NA
Attachment #690353 - Flags: approval-mozilla-esr17?
Attachment #690353 - Flags: approval-mozilla-beta?
Attachment #690353 - Flags: approval-mozilla-b2g18?
Attachment #690353 - Flags: approval-mozilla-aurora?
Comment on attachment 690353 [details] [diff] [review]
simple approach

We'll approve for B2G next week.
Attachment #690353 - Flags: approval-mozilla-esr17?
Attachment #690353 - Flags: approval-mozilla-esr17+
Attachment #690353 - Flags: approval-mozilla-beta?
Attachment #690353 - Flags: approval-mozilla-beta+
Attachment #690353 - Flags: approval-mozilla-aurora?
Attachment #690353 - Flags: approval-mozilla-aurora+
Attachment #690353 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Confirmed crash and assert on ASan build from 2013-01-07, crash on release build 2013-06-04.
Verified fix on build 2013-01-31, m-c
Verified fix on build 2013-01-31, Aurora
Verified fix on beta 19b4
Verified fix on build 2013-01-30, 17.0.2esr
Target Milestone: --- → mozilla21
Whiteboard: [adv-main19+][adv-esr1703+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.