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

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: We should never be reusing a shared inner window: '!currentInner->IsFrozen()', file dom/base/nsGlobalWindow.cpp, line 1776

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

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

I'll look at this.

Comment 3

[Daniel Veditz [:dveditz]]

guessing sec-high?

Comment 4

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

...still on my todo list.

Comment 5

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

Attachment: simple approach

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 690353 [details] [diff] [review]
simple approach

r=me

Comment 7

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

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.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 9

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

The same patch applies to FF18, FF19 and ESR17. (ESR17 with some fuzz).

Comment 10

[Alex Keybl [:akeybl]]

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.

Comment 11

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

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 13

[Alex Keybl [:akeybl]]

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

Comment 14

[Daniel Veditz [:dveditz]]

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

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Given that we're not landing this on 18 which is the final ESR10 build, wontfixing.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

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

Comment 17

[Alex Keybl [:akeybl]]

We're now past the merge, and ready to take a fix on all branches.

Comment 18

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

This needs approval

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 690353 [details] [diff] [review]
simple approach

Al set this previously, so setting to sec‑approval+ again.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/224712fc024e

Comment 21

[Alex Keybl [:akeybl]]

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

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

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

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 690353 [details] [diff] [review]
simple approach

We'll approve for B2G next week.

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b88a7f958592
https://hg.mozilla.org/releases/mozilla-beta/rev/481124f1a662
https://hg.mozilla.org/releases/mozilla-esr17/rev/192823f27a47

Comment 25

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/dd0956a67c27

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

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