538532 - [OOPP] Chrome-process crash [@nsNPAPIPluginStreamListener::OnStartBinding] when plugin process dies during NPP_SetWindow

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

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

See bug 5360202 comment 13 and bug 536020 comment 14.

Comment 1

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

Attachment: part 1, v1: at josh's request, s/nsPluginTag.mEntryPoint/nsPluginTag.mPlugin/. also s/nsPluginHost.mPlugins/nsPluginHost.mPluginTags/ to avoid confusion

Comment 2

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

Attachment: part 2, v1: add nsPluginHost::GetTagForPlugin(nsIPlugin) helper

Comment 3

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

Attachment: part 3, v1: add nsPluginHost::StopPlugin(nsIPlugin)

There may be other sites that can be refactored to use this method, but I didn't see any off-hand.

Comment 4

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

Attachment: part 4, v1: when a plugin crashes, make sure all its instances are stopped ASAP

Comment 5

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

With these patches, when the plugin process crashing at NPP_SetWindow(), valgrind doesn't find any problems.

Comment 6

[Benjamin Smedberg]

I'm a little concerned about all the renaming: remember, we have to port all this back to the branch, where the plugintag stuff hasn't been refactored and we still support the XPCOM/OJI plugin.

Comment 7

[Benjamin Smedberg]

With this patch series you end up finding the plugintag twice (once in PluginCrashed, and another time in StopPlugin. Can StopPlugin either be a method of nsPluginTag, or at least take a nsPluginTag* instead of a nsIPlugin*?

Seems like you might as well combine the patches in part 4 and part 3, since they touch the same code.

Comment 8

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

TBH, for this OOPP bug alone we don't need |StopPlugin()| ... I left it in because I thought that the fix for bug 536020 would end up using it (but I still don't know what's wrong there).  Will reshuffle patches.

Comment 9

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

Attachment: p1 v2: add nsPluginHost::FindTagForPlugin()

Comment 10

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

Attachment: p2 v2: when a plugin crashes, make sure all its instances are stopped ASAP

Simplified per bsmedberg's comments.

Comment 11

[Benjamin Smedberg]

Comment on attachment 420754 [details] [diff] [review]
p2 v2: when a plugin crashes, make sure all its instances are stopped ASAP

Looks ok to me, but josh should confirm.

Comment 12

[Josh Aas]

Comment on attachment 420753 [details] [diff] [review]
p1 v2: add nsPluginHost::FindTagForPlugin()

As a direct replacement for writing out the code multiple times this looks fine.

>   // Let's find the corresponding plugin tag by matching nsIPlugin pointer.
>   // It is going to be used later when we decide whether or not we should delay
>   // unloading NPAPI dll from memory.
>   nsPluginTag * pluginTag = nsnull;
>   if (aPlugin) {
>-    for (pluginTag = mPlugins; pluginTag != nsnull; pluginTag = pluginTag->mNext) {
>-      if (pluginTag->mEntryPoint == aPlugin)
>-        break;
>-    }
>+    pluginTag = FindTagForPlugin(aPlugin);
>     NS_ASSERTION(pluginTag, "Plugin tag not found");
>   }
> 
>   nsPluginInstanceTag * plugin = new nsPluginInstanceTag(pluginTag, aInstance, url.get(), aDefaultPlugin);

This logic look funny though your patch isn't responsible for it. I would think we don't allow aPlugin to be null, and we shouldn't be creating a new nsPluginInstanceTag without a plugintag as the assertion implies.

Comment 13

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

Pushed http://hg.mozilla.org/projects/electrolysis/rev/6064abe2a823
Pushed http://hg.mozilla.org/projects/electrolysis/rev/7bb0a487f2cd

We'll want this on m-c sooner rather than later, but as it hasn't arisen in the wild yet, I'll let it bake on e10s.

Comment 14

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/18500ec42c6f
http://hg.mozilla.org/mozilla-central/rev/68c27494dc5c

Comment 15

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/e508191bbecf
http://hg.mozilla.org/projects/firefox-lorentz/rev/6d7743f544ce

Comment 16

[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 17

[Benjamin Smedberg]

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