863792 - Plugins can re-enter in instantiate, causing plugin tags to lose track of them.

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

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.

Comment 2

[John Schoenick [:johns]]

Attachment: Handle re-entry during plugin instantiation

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.

Comment 3

[John Schoenick [:johns]]

Attachment: Test

Comment 4

[John Schoenick [:johns]]

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

Comment 5

[John Schoenick [:johns]]

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

Comment 6

[John Schoenick [:johns]]

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

Comment 7

[John Schoenick [:johns]]

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

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

Comment 8

[John Schoenick [:johns]]

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

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

Comment 9

[John Schoenick [:johns]]

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.

Comment 10

[John Schoenick [:johns]]

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.

Comment 11

[John Schoenick [:johns]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[John Schoenick [:johns]]

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.

Comment 14

[Alex Keybl [:akeybl]]

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.

Comment 15

[John Schoenick [:johns]]

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

Comment 16

[John Schoenick [:johns]]

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.

Comment 17

[bhavana bajaj [:bajaj]]

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.

Comment 18

[bhavana bajaj [:bajaj]]

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.

Comment 19

[John Schoenick [:johns]]

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

Comment 20

[u279076]

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?

Comment 21

[John Schoenick [:johns]]

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

Comment 22

[u279076]

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

Comment 23

[u279076]

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