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

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
x86
All
Points:
---

Firefox Tracking Flags

(status1.9.2 .4-fixed)

Details

(Whiteboard: [fixed-lorentz])

Attachments

(2 attachments, 4 obsolete attachments)

See bug 5360202 comment 13 and bug 536020 comment 14.
Created attachment 420684 [details] [diff] [review]
part 1, v1: at josh's request, s/nsPluginTag.mEntryPoint/nsPluginTag.mPlugin/.  also s/nsPluginHost.mPlugins/nsPluginHost.mPluginTags/ to avoid confusion
Assignee: nobody → jones.chris.g
Attachment #420684 - Flags: review?
Created attachment 420685 [details] [diff] [review]
part 2, v1: add nsPluginHost::GetTagForPlugin(nsIPlugin) helper
Attachment #420685 - Flags: review?
Created attachment 420686 [details] [diff] [review]
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.
Attachment #420686 - Flags: review?
Created attachment 420687 [details] [diff] [review]
part 4, v1: when a plugin crashes, make sure all its instances are stopped ASAP
Attachment #420687 - Flags: review?(benjamin)
Attachment #420684 - Flags: review? → review?(joshmoz)
Attachment #420685 - Flags: review? → review?(joshmoz)
Attachment #420686 - Flags: review? → review?(joshmoz)
With these patches, when the plugin process crashing at NPP_SetWindow(), valgrind doesn't find any problems.

Comment 6

8 years ago
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

8 years ago
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.
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.
Created attachment 420753 [details] [diff] [review]
p1 v2: add nsPluginHost::FindTagForPlugin()
Attachment #420684 - Attachment is obsolete: true
Attachment #420685 - Attachment is obsolete: true
Attachment #420753 - Flags: review?(joshmoz)
Attachment #420684 - Flags: review?(joshmoz)
Attachment #420685 - Flags: review?(joshmoz)
Created attachment 420754 [details] [diff] [review]
p2 v2: when a plugin crashes, make sure all its instances are stopped ASAP

Simplified per bsmedberg's comments.
Attachment #420686 - Attachment is obsolete: true
Attachment #420687 - Attachment is obsolete: true
Attachment #420754 - Flags: review?(benjamin)
Attachment #420687 - Flags: review?(benjamin)
Attachment #420686 - Flags: review?(joshmoz)

Comment 11

8 years ago
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.
Attachment #420754 - Flags: review?(joshmoz)
Attachment #420754 - Flags: review?(benjamin)
Attachment #420754 - Flags: review+
Keywords: testcase-wanted

Comment 12

8 years ago
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.
Attachment #420753 - Flags: review?(joshmoz) → review+

Updated

8 years ago
Attachment #420754 - Flags: review?(joshmoz) → review+
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.
Blocks: 539055
Whiteboard: [land m-c]

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/18500ec42c6f
http://hg.mozilla.org/mozilla-central/rev/68c27494dc5c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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

8 years ago
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
status1.9.2: --- → .4-fixed
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.