Closed Bug 975098 Opened 6 years ago Closed 6 years ago

CheckPluginStopEvent might not be queued if a object/embed/applet is removed from a document inside NPP_New re-entrance

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: johns, Assigned: johns)

References

Details

Attachments

(2 files)

khuey noticed a non-reproducible case of flash living beyond a document going inactive. Reading through the code for how that might happen, I noticed a few places (e.g. [1]) where we do something like:
> if (mInstanceOwner) { StopPluginInstance {or} QueueCheckPluginStopEvent }

This wouldn't work if these places can occur inside InstantiatePluginInstance->NPP_New->NPN_Invoke->Arbitrary script

Anything checking if a plugin may need to be stopped needs to check (mInstanceOwner || mInstantiating). This doesn't apply to calling UnloadObject(), since changing tag state clears mInstantiating and the re-entrance guard destroys the superseded plugin [2]

[1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#704

[2] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#822
mInstantiating is already a re-entrance guard for this, we just needed to fix the cases where were were using if (mInstanceOwner) to guard against unnecessary CheckPluginStopEvents

As a bonus, clear mInstantiating if StopPluginInstance() is called, so the instantiate aborts if something tries to stop a plugin inside starting a plugin. This almost certainly happens all the time, becuase plugins.
Bonus bug! InstantiatPluginInstance was firing PluginInstantiate at wherever the plugin is *after* spawning, which may not be the document it was spawned in.

Previously UnbindFromTree() would only check mInstanceOwner, see no plugin, and call UnloadObject(), causing the spawn to abort when instantiate returned. With the other patch, we (correctly) queue an event to despawn the new plugin, allowing it to be re-attached to the tree later. But that means plugins can be in no document when Instantiate returns, without aborting spawn, causing MOZ_ASSERT to crash us when we fire an event at a null document.
Attachment #8380006 - Flags: review?(benjamin)
Comment on attachment 8380075 [details] [diff] [review]
Fire PluginInstantiated at the document the plugin was instantiated in, even if it moves documents inside NPP_New

This file is trying oh-so-hard to get within 80 characters
Attachment #8380075 - Flags: review?(benjamin)
Attachment #8380006 - Flags: review?(benjamin) → review+
Comment on attachment 8380075 [details] [diff] [review]
Fire PluginInstantiated at the document the plugin was instantiated in, even if it moves documents inside NPP_New

I so want to not allow people to adopt plugin nodes across documents. But also I'm a little unclear on why PluginInstantiated is the event we care about here: isn't that event only used by the CtP frontend code? Comment 2 seems to be about something else entirely.
Flags: needinfo?(jschoenick)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 8380075 [details] [diff] [review]
> Fire PluginInstantiated at the document the plugin was instantiated in, even
> if it moves documents inside NPP_New
> 
> I so want to not allow people to adopt plugin nodes across documents. But
> also I'm a little unclear on why PluginInstantiated is the event we care
> about here: isn't that event only used by the CtP frontend code? Comment 2
> seems to be about something else entirely.

The problem with PluginInstantiated is that it's fired inside InstantiatePluginInstance, after calling into NPP_New. If we re-enter in NPP_New and lose our document, we'll try to fire the event at null and crash. This patch just saves the doc we were in when we instantiated so we know where the event should actually be going.

This worked before this bug because removing the plugin from the document inside NPP_New would cause UnbindFromTree() to incorrectly think we had no plugin, and call UnloadObject(). The re-entrance check after NPP_New would see we re-entered and destroyed the plugin, and we'd abort before firing PluginInstantiated.
Flags: needinfo?(jschoenick)
Attachment #8380075 - Flags: review?(benjamin) → review+
Blocks: 981422
https://hg.mozilla.org/mozilla-central/rev/25de9f318261
https://hg.mozilla.org/mozilla-central/rev/65d3cda9a1a9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 1013972
No longer depends on: 1013972
You need to log in before you can comment on or make changes to this bug.