552866 - PluginModuleParent::NotifyPluginCrashed can still be run from a nested context

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

See http://crash-stats.mozilla.com/report/index/72842f28-cc99-464e-abf7-befe92100316

Following in the grand traditions of bug 546035, bug 550026, and bug 552014, we have another way this task can be processed in a nested loop.  Problem is, this backtrace doesn't allow for full diagnosis, but there are two options AFAICT.  In both cases, the problem is calling PluginModuleParent::Close() from a nested context.

 (1) Plugin was killed by the hang detector.  The CleanupFromTimeoutTask generated by hangs doesn't guard against running in a nested context and calls Close().

 (2) Plugin crashed during the nested loop, some NPAPI call failed, and nsNPAPIPlugin called NP_Shutdown from the nested loop (which also goes into Close()).

We can fix both cases by resurrecting the (ugly :S) patch from bug 550026 that guarded against running NotifyPluginCrashed() in a nested context.  C'est la guerre.

Addendum: this backtrace concerns me in that hints at a modal dialog keeping FF in a nested loop after the plugin crashed.  Is it possible for a web app (and hence plugin) to throw a modal dialog that can only be dismissed by script?  I'm guessing not, but if so, then we have a case where a plugin can perma-freeze the FF UI after crashing.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't run NotifyPluginCrashed from a nested context, srsly

This last hack was the tipping point for me, filed bug 552869 on the broken architecture.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #1)
> This last hack was the tipping point for me, filed bug 552869 on the broken
> architecture.

(Though still don't think it should block OOPP.)

Comment 3

[Benjamin Smedberg]

Comment on attachment 432982 [details] [diff] [review]
Don't run NotifyPluginCrashed from a nested context, srsly

ugh, unhappiness but ok

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/ca6d56e7db4a

Comment 5

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/719f24467d4d

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 7

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430