Closed Bug 863792 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

21 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox20 --- wontfix
firefox21 + fixed
firefox22 + fixed
firefox23 + fixed
firefox-esr17 --- wontfix
b2g18 --- unaffected

People

(Reporter: johns, Assigned: johns)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

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.
Blocks: 859099
Duplicate of this bug: 856171
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.
Attachment #739717 - Flags: review?(benjamin)
Attached patch TestSplinter Review
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
Attachment #739717 - Flags: review?(benjamin) → review?(joshmoz)
Attachment #739718 - Flags: review?(joshmoz)
Attachment #739717 - Flags: review?(joshmoz) → review+
Attachment #739718 - Flags: review?(joshmoz) → review+
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.
Attachment #739717 - Flags: approval-mozilla-beta?
Attachment #739717 - Flags: approval-mozilla-aurora?
Attachment #739718 - Flags: approval-mozilla-beta?
Attachment #739718 - Flags: approval-mozilla-aurora?
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.
Attachment #739717 - Flags: checkin+
Attachment #739717 - Flags: approval-mozilla-beta?
Attachment #739717 - Flags: approval-mozilla-aurora?
Depends on: 854082
Attachment #739718 - Flags: checkin+
Attachment #739718 - Flags: approval-mozilla-beta?
Attachment #739718 - Flags: approval-mozilla-aurora?
Attachment #739717 - Flags: checkin+
Attachment #739718 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/557f1cfca69c
https://hg.mozilla.org/mozilla-central/rev/39b0b62b8cd2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
Attachment #739717 - Flags: approval-mozilla-aurora?
Attachment #739718 - Flags: approval-mozilla-aurora?
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.
Attachment #739717 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #739718 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #739717 - Flags: approval-mozilla-beta?
Attachment #739718 - Flags: approval-mozilla-beta?
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.
Attachment #739717 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #739718 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
(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 :(
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-].
Keywords: verifyme
After some overnight testing we were not able to come up with a reproducible testcase of verifying this bug; thus marking as [qa-].
Keywords: verifyme
Whiteboard: [qa-]
Blocks: 852721
You need to log in before you can comment on or make changes to this bug.