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

Spun off from bug 859099, which is the same root issue, but is specific to the XP crashes now occurring on 21+, whereas the root issue is present on all branches/platforms.

If we re-enter here:
http://dxr.mozilla.org/mozilla-central/content/base/src/nsObjectLoadingContent.cpp#l743

Or possibly in the layout flush above, then we might switch the tag away from type plugin, without stopping the instantiating plugin, which is not yet in mInstanceOwner. Up until FF21 this was mostly harmless due to this check:
http://dxr.mozilla.org/mozilla-central/content/base/src/nsObjectLoadingContent.cpp#l680

Which cleans up plugins that we lost track of. However in 21+, that reaches TeardownProtoChain, which is pure virtual at this point, causing an abort.

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

Blow away mInstantiating if we unload the current content, and have instantiate check if that happened and throw out the obsolete plugin. Bonus re-entry check for layout flushing.

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

Comment on attachment 739717 [details] [diff] [review]
Handle re-entry during plugin instantiation

Josh would probably be better to review this as he's touched this code

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

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

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

For beta: https://tbpl.mozilla.org/?tree=Try&rev=517c13193ec8

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

Comment on attachment 739718 [details] [diff] [review]
Test

https://hg.mozilla.org/integration/mozilla-inbound/rev/d226a39b4181

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

Comment on attachment 739717 [details] [diff] [review]
Handle re-entry during plugin instantiation

https://hg.mozilla.org/integration/mozilla-inbound/rev/9423207656dd

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

Comment on attachment 739717 [details] [diff] [review]
Handle re-entry during plugin instantiation

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug has existed since dawn of time, but bug 851378 caused it to be a top-crasher in bug 859099

User impact if declined:
Top crash (Bug 859099)

Testing completed (on m-c, etc.):
On m-c, has testcase

Risk to taking this patch (and alternatives if risky):
Moderate. Plugins are very issue-prone, but worse-case scenario is we exchange one top crash for another...

String or IDL/UUID changes made by this patch:
None

Note: The crash doesn't happen on aurora for complicated reasons, but it is affected by the root bug. We could skip aurora, but it's probably better to get more testing sooner.

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

Comment on attachment 739717 [details] [diff] [review]
Handle re-entry during plugin instantiation

Well so much for getting this in before tomorrow:

https://hg.mozilla.org/integration/mozilla-inbound/rev/221e325b028c

Bug 854082 was backed out for hitting assertions from another broken test, but this bug triggers that bug so has to land after it yay.

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

Attempt 2, relevant try run: https://tbpl.mozilla.org/?tree=Try&rev=dbfaf4b211bb

https://hg.mozilla.org/integration/mozilla-inbound/rev/557f1cfca69c
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b0b62b8cd2

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

https://hg.mozilla.org/mozilla-central/rev/557f1cfca69c
https://hg.mozilla.org/mozilla-central/rev/39b0b62b8cd2

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

Comment on attachment 739717 [details] [diff] [review]
Handle re-entry during plugin instantiation

Per conversation with bbajaj, requesting uplift to aurora to get more testing before considering for beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug has existed since dawn of time, but bug 851378 caused it to be a top-crasher in bug 859099

User impact if declined:
Top crash (Bug 859099)

Testing completed (on m-c, etc.):
On m-c, has testcase

Risk to taking this patch (and alternatives if risky):
Moderate. Plugins are very issue-prone, but worse-case scenario is we exchange one top crash for another...

String or IDL/UUID changes made by this patch:
None

Note: The crash doesn't happen on aurora for complicated reasons, but it is affected by the root bug, and any fallout would be the same.

	Alex Keybl [:akeybl]
	Comment 14 • 12 years ago

Comment on attachment 739717 [details] [diff] [review]
Handle re-entry during plugin instantiation

This will hopefully resolve bug 859099. Rushing for Aurora 22 in preparation for possible landing to Beta 21.

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/50e76258e71d
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c5a9fdda49e

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

Comment on attachment 739717 [details] [diff] [review]
Handle re-entry during plugin instantiation

This is on aurora + nightly now, nominating for beta so release can decide if/when we want to uplift it.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug has existed since dawn of time, but bug 851378 caused it to be a top-crasher in bug 859099

User impact if declined:
Top crash (Bug 859099)

Testing completed (on m-c, etc.):
On m-c and m-a, has testcase.

Risk to taking this patch (and alternatives if risky):
Moderate. Plugins are very issue-prone. The patch is fairly small and most likely correct, but exposing latent issues in other plugin code happens all the time (see: 859099)

*Behavioral* bustage seems pretty unlikely, however, unless sites are depending on a very very broken behavior out of the <object> tag. Worst case situation would probably be trading one topcrash for another.

String or IDL/UUID changes made by this patch:
None

Note: The crash doesn't happen on aurora for complicated reasons, but it is affected by the root bug, and any fallout would be the same.

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

Although this is medium risk and given we are not left with alternatives here for Fx21 based on discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=859099#c53, we are going to take the fwd fix for Fx21 beta 6 and hoping the top-crasher is fixed.We are also trying to see if we can reproduce the crash in 859099 by any bug Url's to be more confident around testing here.

Also based on the above comment, we are not expecting and behavioral bustage around plugin's and the worst case is we could exchange it with another crasher.We have also let this bake on aurora to make sure they were no fallouts & as of now we have not seen any regressions.

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

Comment on attachment 739718 [details] [diff] [review]
Test

Please land this on mozilla-beta before tomorrow noon for this to make it into our next beta.

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

https://hg.mozilla.org/releases/mozilla-beta/rev/1429de31b9a1
https://hg.mozilla.org/releases/mozilla-beta/rev/65581cf79180

	u279076
	Comment 20 • 12 years ago

As with bug 859099 comment 55, is there anything QA can do to discretely verify this is fixed or is this more speculative in nature?

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

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #20)
> As with bug 859099 comment 55, is there anything QA can do to discretely
> verify this is fixed or is this more speculative in nature?

Bug 859099 is a symptom of leaking a running plugin, and this bug is one specific case in which we can do so. Verifying that the testcase doesn't crash or assert when plugins are in-process verifies the issue fairly well, though ideally we'd have a testcase that triggered the issue for OOP plugins as well :(

	u279076
	Comment 22 • 12 years ago

Thanks John. I'm tentatively adding verifyme to this bug to see if we can come up with something for 21.0b6. However this bug might end up [qa-].

	u279076
	Comment 23 • 12 years ago

After some overnight testing we were not able to come up with a reproducible testcase of verifying this bug; thus marking as [qa-].