Timothy Nikkel (:tnikkel) Reporter
	Description • 12 years ago

I made a clean debug mac build of rev e23e43a2c14e

If I load
http://congressoamericano.blogspot.ca/
wait for the music to start, then click on the stop button I get a crash. The stack goes from plugin destruction into running a script which plays with things and that causes the crash.

This page hasn't had a problem for me before, so I suspect something changed, either the website or something on mozilla-central.

This doesn't seem to crash the latest nightly for whatever reason.

	Steven Michaud [:smichaud] (Retired)
	Comment 1 • 12 years ago

> https://hg.mozilla.org/mozilla-central/rev/e23e43a2c14e

This is still the tip.  So whatever change triggered these crashes will presumably appear in tomorrow's mozilla-central nightly.

Please wait til it appears and test with it.

	Steven Michaud [:smichaud] (Retired)
	Comment 2 • 12 years ago

Tinderbox opt build corresponding to rev e23e43a2c14e:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64/1363637255/firefox-22.0a1.en-US.mac.dmg

Also try testing with this.

So will I.

	Steven Michaud [:smichaud] (Retired)
	Comment 4 • 12 years ago

I can't repro with the build I posted in comment #2.

I tested on OS X 10.8.2.

	Steven Michaud [:smichaud] (Retired)
	Comment 5 • 12 years ago

But I do see crashes testing with the tinderbox debug build corresponding with rev e23e43a2c14e:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1363637255/firefox-22.0a1.en-US.mac64.dmg

bp-4c5b9c1d-53ba-4271-ba1f-afa362130318
bp-23d13f61-fd74-415c-b2d6-be0be2130318

They usually happen when I click on the Play button.

	Steven Michaud [:smichaud] (Retired)
	Comment 6 • 12 years ago

But the crashes also happen with today's mozilla-central-debug nightly:

bp-63ae03b0-a2fc-40cc-b273-1ac392130318

So someone will need to establish the regression range using mozilla-central-debug nightlies.

	John Schoenick [:johns] Assignee
	Comment 7 • 12 years ago

nsDocument calls EnumerateFreezables in order to send notifications to all relevant elements that the document has gone inactive, but nsObjectLoadingContent spins the plugin code in that notification, which re-enters, which runs arbitrary script, which appends a freezable element to the document, which modifies the hash table we're enumerating.

It might be possible to achieve arbitrary memory corruption with this :-/

	Andrew McCreight [:mccr8]
	Comment 8 • 12 years ago

Sounds like possible sec-critical.

	John Schoenick [:johns] Assignee
	Comment 11 • 12 years ago

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

Just use CheckPluginStopEvent when documents go inactive as well. We already use this for UnbindFromTree, and since nsDocument is non-reentrant at this point this seems like the sane solution if no tests explode.

	John Schoenick [:johns] Assignee
	Comment 12 • 12 years ago

Comment on attachment 726434 [details] [diff] [review]
Add scary warning comment

Add a warning comment, but split out since we wont want to land it until the fix is shipped

	John Schoenick [:johns] Assignee
	Comment 13 • 12 years ago

Pushed to try folded with other misc plugin patches (it's a pretty non-obvious issue):

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

	John Schoenick [:johns] Assignee
	Comment 14 • 12 years ago

Marking b2g unaffected as this requires a spawned plugin

	John Schoenick [:johns] Assignee
	Comment 15 • 12 years ago

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

This looks good on try -- the bc failure was from one of the other patches in that run.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Unknown, has existed for a long while

User impact if declined:
sec bug - fairly easy to trigger memory corruption

Testing completed (on m-c, etc.):
None, will land on m-c prior to branches

Risk to taking this patch (and alternatives if risky):
Low risk

String or UUID changes made by this patch:
None

	John Schoenick [:johns] Assignee
	Comment 16 • 12 years ago

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
If the issue is noticed, the exploit is fairly trivial, but it is a somewhat obscure re-entrance issue.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The warning comment about re-entry was split into another patch that can land later.

Which older supported branches are affected by this flaw?
All current branches -- but the issue requires a running plugin, so not relevant to b2g.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Rebasing on branches is trivial

How likely is this patch to cause regressions; how much testing does it need?
Ordering of plugin events and teardown has always been finicky, but this changes "document has gone inactive" behavior to be identical to "removed from document" behavior for plugins, and should be fairly low risk.

	John Schoenick [:johns] Assignee
	Comment 17 • 12 years ago

Comment on attachment 726434 [details] [diff] [review]
Add scary warning comment

[Security approval request comment]
This just adds a comment to warn about the non-obvious re-entrance risk, and should land once the fix is shipped.

	Al Billings [:abillings - ex-MoCo]
	Comment 18 • 12 years ago

We're too close to release to take this now, AFAIK. I want to get release management commentary but I expect that we won't approve this for a couple of weeks until after Firefox 20 ships.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 19 • 12 years ago

It's definitely too late for FF20, marking tracking accordingly.  This is internally reported and if Al isn't concerned about it sitting until a couple weeks into FF21 let's do that and buy some proper bake time.

	Al Billings [:abillings - ex-MoCo]
	Comment 20 • 12 years ago

For these same reasons, I'd prefer to wait until after we ship 20. I'd say that any time from 4/16 (end of second week of the cycle) on would be good since, if noticed, an exploit wouldn't be hard per comment 16.

	John Schoenick [:johns] Assignee
	Comment 21 • 12 years ago

This is flagged to be checked in this week but still lacks branch approvals -- are we waiting on an m-c landing first, or do we want this to land everywhere?

	Al Billings [:abillings - ex-MoCo]
	Comment 22 • 12 years ago

Please land it on m-c first and mark the patch (or patches) for branch as necessary after. It doesn't need to land everywhere at once.

	bhavana bajaj [:bajaj]
	Comment 23 • 12 years ago

All the branch patches will be approved, once this has landed on m-c .

	John Schoenick [:johns] Assignee
	Comment 24 • 12 years ago

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

https://hg.mozilla.org/integration/mozilla-inbound/rev/021bca10985b

	John Schoenick [:johns] Assignee
	Comment 25 • 12 years ago

argh, bugzilla

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/021bca10985b

	bhavana bajaj [:bajaj]
	Comment 27 • 12 years ago

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

Adding Matt as QA contact to help verify this on trunk & branches once this lands.

	John Schoenick [:johns] Assignee
	Comment 28 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8aeaed19ddd

Required some tweaks to rebase on beta/esr17, so waiting on this build to finish before pushing there...

	John Schoenick [:johns] Assignee
	Comment 29 • 12 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/5ec90ea60b60

	John Schoenick [:johns] Assignee
	Comment 30 • 12 years ago

https://hg.mozilla.org/releases/mozilla-esr17/rev/214646821a0d

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 31 • 12 years ago

I'm not able to repro on any debug builds on or around reported date. Also, try builds don't appear to be available that far back either.

So I guess I'd like to ask Timothy/Steven to verify that an affected config no longer crashes, if that's possible.

	Steven Michaud [:smichaud] (Retired)
	Comment 32 • 12 years ago

I can no longer reproduce this bug's crashes using the build I tested with in comment #6.  I assume the website's changed.

	Timothy Nikkel (:tnikkel) Reporter
	Comment 33 • 12 years ago

Yeah, I couldn't reproduce a crash either. I think the site must have changed as well, because I had been testing with this site for a while before the first crash happen, so it seems a site changed is what made this show up in the first place, it makes sense that a site change with make it go away too.

	Timothy Nikkel (:tnikkel) Reporter
	Comment 34 • 12 years ago

Or a similar comment saying the same but with correct grammar. :)

	u279076
	Comment 35 • 12 years ago

Given we've lost our testcase (due to a website change) I don't think we'll be able to truly verify this is fixed. For this reason I am tagging this bug qa-. Please re-add the verifyme keyword if circumstances change which allow us to test the fix.

	John Schoenick [:johns] Assignee
	Comment 36 • 12 years ago

Not as useful as a live site using flash, but this test case reliably causes the crash using the test plugin, which should at least be helpful in ensuring its properly fixed on branches.

	u279076
	Comment 37 • 12 years ago

Thanks John.

Matt can you please verify this is fixed using John's testcase?

	John Schoenick [:johns] Assignee
	Comment 38 • 12 years ago

For landing along with scary warning comment once this is fixed everywhere

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 39 • 12 years ago

John, thanks for going the extra mile and making the test case!

Only issue is that I can't get it to crash for me. I'm using debug builds, from 3/18-4/18, all affected branches - I just get an iframe and the missing plugin UI. Nothing crashes.

Any ideas? Thanks.

	John Schoenick [:johns] Assignee
	Comment 40 • 12 years ago

(In reply to Matt Wobensmith from comment #39)
> John, thanks for going the extra mile and making the test case!
> 
> Only issue is that I can't get it to crash for me. I'm using debug builds,
> from 3/18-4/18, all affected branches - I just get an iframe and the missing
> plugin UI. Nothing crashes.
> 
> Any ideas? Thanks.

The test needs the nptest plugin to be a co-conspirator in behaving badly, which I think was changed to not be loaded by default in debug builds. You can fix this by copying/symlinking dist/plugins to dist/bin/browser/plugins. Make sure about:plugins shows a plugin claiming "application/x-test"

You can also load nptest into release builds by grabbing a version of it from a similarly-versioned debug build (it never changes much) and putting it into some_test_profile/plugins/ or (I think?) install_dir/plugins/, though the latter may have been disabled.

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 41 • 12 years ago

Got it. I see a crash now, but it affects the fixed builds too. This is Mac.

Needs more investigation. John mentioned to me that he will take another look at it shortly.

	John Schoenick [:johns] Assignee
	Comment 42 • 12 years ago

(In reply to Matt Wobensmith from comment #41)
> Needs more investigation. John mentioned to me that he will take another
> look at it shortly.

I borrowed someone's OS X build and gave this a shot, but wasn't able to reproduce the crash. I don't have a proper OS X environment setup at the moment -- before I try to dig into that, do you think you can get a backtrace w/symbols of the crash you're seeing?

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 43 • 12 years ago

OK, working with John, I see that I have an unrelated crash due to a faulty plugin or issue therein. However, this bug is fixed.

Verified crash in m-c, 2013-03-19
Verified fix in m-c 23.0a1, 2013-04-25
Verified fix in Aurora, 22.0a2 2013-04-25
Verified fix in 21.0, 2013-04-24
Verified fix in 17.0.5esrpre, 2013-04-22

	u279076
	Comment 44 • 12 years ago

Thanks Matt!

	John Schoenick [:johns] Assignee
	Comment 45 • 11 years ago

Un-bitrotted

	John Schoenick [:johns] Assignee
	Comment 46 • 11 years ago

Comment on attachment 726434 [details] [diff] [review]
Add scary warning comment

https://hg.mozilla.org/integration/mozilla-inbound/rev/23dfc36f99d4

	John Schoenick [:johns] Assignee
	Comment 47 • 11 years ago

Comment on attachment 8379260 [details] [diff] [review]
Test. r=bsmedberg

Pushed test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13cc8a7ee925

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 48 • 11 years ago

I backed this (and everything else from that push to inbound) out in http://hg.mozilla.org/integration/mozilla-inbound/rev/128cbf1edc40 due to various Java/plugin related failures they caused:
Crashtest failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35003251&tree=Mozilla-Inbound
XPCShell failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35003355&tree=Mozilla-Inbound

	John Schoenick [:johns] Assignee
	Comment 49 • 11 years ago

Attempt 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9e44139023
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d186c979373

Extensive try run since this push burned the tree before:
https://tbpl.mozilla.org/?tree=Try&rev=b656bdd77ce1

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 50 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5c9e44139023
https://hg.mozilla.org/mozilla-central/rev/0d186c979373