Closed Bug 538532 Opened 14 years ago Closed 14 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

x86
All
defect
Not set
normal

Tracking

(status1.9.2 .4-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: cjones, Assigned: cjones)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(2 files, 4 obsolete files)

See bug 5360202 comment 13 and bug 536020 comment 14.
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?
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.
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.
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.
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)
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 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+
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+
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: LorentzBeta1
Whiteboard: [land m-c]
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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.