Closed Bug 536437 Opened 10 years ago Closed 10 years ago

[OOPP] Flash crash [@mozilla::plugins::PStreamNotifyParent::Call__delete__]

Categories

(Core :: Plug-ins, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: benjamin)

References

Details

Attachments

(2 files, 2 obsolete files)

http://crash-stats.mozilla.com/report/index/bp-f12be828-4ac0-4c1b-b6cb-b5b252091221
http://crash-stats.mozilla.com/report/index/bp-3bc751a4-05e0-49c9-923b-12b242091221
http://crash-stats.mozilla.com/report/index/bp-afca4c82-327b-4f14-9e44-5abe32091221

From bsmedberg:
The possibility is that the implementation of NPP_GetURLNotify is calling
NPN_URLNotify within the same stack frame, but then returning a failure error
code. That combination would cause the crash listed here.
Can't easily repro this on hulu.
Blocks: OOPP
Attached patch test case that triggers crash (obsolete) — Splinter Review
I'm pretty sure the malformed-data:-URI test is triggering the crash, but I'd like to keep the other tests here too.
valgrind did most of the work for me.

==13663== Invalid read of size 4
==13663==    at 0x683094A: mozilla::plugins::PStreamNotifyParent::Call__delete__(mozilla::plugins::PStreamNotifyParent*, short const&) (PStreamNotifyParent.cpp:63)
==13663==    by 0x67F31FF: mozilla::plugins::PluginInstanceParent::AnswerPStreamNotifyConstructor(mozilla::plugins::PStreamNotifyParent*, nsCString const&, nsCString const&, bool const&, nsCString const&, bool const&, short*) (PluginInstanceParent.cpp:289)
==13663==    by 0x681D18D: mozilla::plugins::PPluginInstanceParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginInstanceParent.cpp:893)
==13663==    by 0x68144B8: mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginModuleParent.cpp:415)
==13663==    by 0x680DF1B: mozilla::ipc::RPCChannel::DispatchIncall(IPC::Message const&) (RPCChannel.cpp:347)
==13663==    by 0x680DE34: mozilla::ipc::RPCChannel::Incall(IPC::Message const&, unsigned long) (RPCChannel.cpp:332)
==13663==    by 0x680D55E: mozilla::ipc::RPCChannel::Call(IPC::Message*, IPC::Message*) (RPCChannel.cpp:180)
==13663==    by 0x6824E00: mozilla::plugins::PPluginScriptableObjectParent::CallInvoke(long const&, nsTArray<mozilla::plugins::Variant> const&, mozilla::plugins::Variant*, bool*) (PPluginScriptableObjectParent.cpp:232)
==13663==    by 0x680104C: mozilla::plugins::PluginScriptableObjectParent::ScriptableInvoke(NPObject*, void*, _NPVariant const*, unsigned int, _NPVariant*) (PluginScriptableObjectParent.cpp:401)
==13663==    by 0x65B9C1D: CallNPMethodInternal(JSContext*, JSObject*, unsigned int, long*, long*, int) (nsJSNPRuntime.cpp:1429)
==13663==    by 0x65B9DB5: CallNPMethod(JSContext*, JSObject*, unsigned int, long*, long*) (nsJSNPRuntime.cpp:1479)
==13663==    by 0x78A9C99: js_Invoke (jsinterp.cpp:1376)
==13663==  Address 0x110e36d8 is 24 bytes inside a block of size 40 free'd
==13663==    at 0x4C24A7A: operator delete(void*) (vg_replace_malloc.c:346)
==13663==    by 0x67F4963: mozilla::plugins::StreamNotifyParent::~StreamNotifyParent() (StreamNotifyParent.h:48)
==13663==    by 0x67F3241: mozilla::plugins::PluginInstanceParent::DeallocPStreamNotify(mozilla::plugins::PStreamNotifyParent*) (PluginInstanceParent.cpp:297)
==13663==    by 0x6830B98: mozilla::plugins::PStreamNotifyParent::Call__delete__(mozilla::plugins::PStreamNotifyParent*, short const&) (PStreamNotifyParent.cpp:97)
==13663==    by 0x67F3CB0: mozilla::plugins::PluginInstanceParent::NPP_URLNotify(char const*, short, void*) (PluginInstanceParent.cpp:654)
==13663==    by 0x67F9242: mozilla::plugins::PluginModuleParent::NPP_URLNotify(_NPP*, char const*, short, void*) (PluginModuleParent.cpp:311)
==13663==    by 0x6598CEE: nsNPAPIPluginStreamListener::CallURLNotify(short) (nsNPAPIPluginInstance.cpp:318)
==13663==    by 0x65988BF: nsNPAPIPluginStreamListener::~nsNPAPIPluginStreamListener() (nsNPAPIPluginInstance.cpp:235)
==13663==    by 0x6597FEA: nsNPAPIPluginStreamListener::Release() (nsNPAPIPluginInstance.cpp:184)
==13663==    by 0x65969D5: nsCOMPtr<nsIPluginStreamListener>::~nsCOMPtr() (nsCOMPtr.h:510)
==13663==    by 0x6590331: MakeNewNPAPIStreamInternal(_NPP*, char const*, char const*, eNPPStreamTypeInternal, int, void*, unsigned int, char const*, unsigned char) (nsNPAPIPlugin.cpp:698)
==13663==    by 0x6591765: mozilla::plugins::parent::_geturlnotify(_NPP*, char const*, char const*, void*) (nsNPAPIPlugin.cpp:1062)
==13663== 
###!!! ABORT: actor has been delete'd: file PStreamNotifyParent.cpp, line 64

What's happening is that the nsNPAPIStreamListener is calling NPP_URLNotify() even in error conditions, which invokes the PStreamNotify dtor.  However, because there's an error, the cleanup code in PluginInstanceParent::AnswerPStreamNotifyConstructor also invokes the destructor.

Unfortunately, it's possible to error out before the nsNPAPIStreamListener has been created, so there's not an easy way to tell in PluginInstanceParent whether it should send the dtor.  I think it'd be better if it just never tried, and instead relied on nsNPAPI* code.  This is closer to the in-process behavior too.
There's a test of a bad javascript: URI in the file, but I commented it out because it triggers a mochitest failure that appears to be hard to suppress.
Assignee: nobody → jones.chris.g
Attachment #418933 - Attachment is obsolete: true
Worth noting: if creating the stream fails before the nsNPAPIStreamListener has been created, then the PStreamNotify will be orphaned until its creating PluginInstance is destroyed.  But it will be cleaned up when that happens.
Attachment #418958 - Flags: review?(benjamin)
This slightly more complicated version makes me happier because the "normal" case where NPN_GetURLNotify fails and doesn't call NPP_URLNotify doesn't leave actors sitting around, and more importantly we don't end up with unexpected NPP_URLNotify calls much later in life when the instance and urlnotify actors are destroyed.
Assignee: jones.chris.g → benjamin
Attachment #419482 - Flags: review?(jones.chris.g)
(In reply to comment #6)
> and more importantly we don't end up with unexpected
> NPP_URLNotify calls much later in life when the instance and urlnotify actors
> are destroyed.

That wouldn't have happened.
Comment on attachment 419482 [details] [diff] [review]
Delete the PStreamNotifyParent only if it hasn't already been deleted, rev. 1

>Bug 536437 - work around a bug in the Mozilla plugin host where NPN_GetURLNotify can return an error and also call NPP_URLNotify, r?cjones
>
>diff --git a/dom/plugins/PluginInstanceParent.cpp b/dom/plugins/PluginInstanceParent.cpp
>--- a/dom/plugins/PluginInstanceParent.cpp
>+++ b/dom/plugins/PluginInstanceParent.cpp
>@@ -265,33 +265,39 @@ bool
> PluginInstanceParent::AnswerPStreamNotifyConstructor(PStreamNotifyParent* actor,
>                                                      const nsCString& url,
>                                                      const nsCString& target,
>                                                      const bool& post,
>                                                      const nsCString& buffer,
>                                                      const bool& file,
>                                                      NPError* result)
> {
>+    bool destructionGuard = false;

Nit: this isn't guarding anything by itself, it's a flag.  |isDead| or something like that seems like a more descriptive name.

>diff --git a/dom/plugins/StreamNotifyParent.h b/dom/plugins/StreamNotifyParent.h
>--- a/dom/plugins/StreamNotifyParent.h
>+++ b/dom/plugins/StreamNotifyParent.h
>@@ -43,15 +43,34 @@
> 
> namespace mozilla {
> namespace plugins {
> 
> class StreamNotifyParent : public PStreamNotifyParent
> {
>   friend class PluginInstanceParent;
> 
>-  StreamNotifyParent() { }
>+  StreamNotifyParent()
>+    : mStackDestructionGuard(NULL)
>+  { }
>+  ~StreamNotifyParent() {
>+    if (mStackDestructionGuard)
>+      *mStackDestructionGuard = true;
>+  }
>+
>+public:
>+  // If we are destroyed within the call to NPN_GetURLNotify, notify the caller
>+  // so that we aren't destroyed again. see bug 536437.
>+  void SetStackDestructionGuard(bool* guard) {
>+    mStackDestructionGuard = guard;
>+  }
>+  void ClearStackDestructionGuard() {
>+    mStackDestructionGuard = NULL;
>+  }
>+
>+private:
>+  bool* mStackDestructionGuard;
> };
> 

Nit: again this bool isn't guarding anything, and it doesn't have anything to do with the stack being destroyed.  s/StackDestructionGuard/WasDestroyedFlag/ or something similar.
Attachment #419482 - Flags: review?(jones.chris.g) → review+
Is this Linux only ? or are other platforms affected ?

Is this ready now for checkin ?  Not been testing OOPP due to crashes.
All platforms. I have to attach a new patch with the requested review changes, which I hope to do today (I have been on vacation).
OS: Linux → All
Version: unspecified → Trunk
Yes, m-c was closed and I wanted to get testing. The e10s orange is concerning!
Duplicate of this bug: 538250
If it helps, see (duplicate) Bug 538250 comment 2 for STR.
That bug also has a reduced testcase: attachment 420417 [details] [diff] [review]
In debug builds, this bug causes a segfault.  In opt builds, you might see

Crash [ @ linux-gate.so @0x422 ] and "ABORT: actor has been delete'd: file PStreamNotifyParent.cpp line 64"
This caused leaks on e10s:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of PStreamNotifyParent with size 20 bytes

TEST-UNEXPECTED-FAIL | plugin process 3268 | automationutils.processLeakLog() | leaked 1 instance of ChildNPObject with size 8 bytes
TEST-UNEXPECTED-FAIL | plugin process 3268 | automationutils.processLeakLog() | leaked 1 instance of PStreamNotifyChild with size 20 bytes
TEST-UNEXPECTED-FAIL | plugin process 3268 | automationutils.processLeakLog() | leaked 1 instance of nsStringBuffer with size 8 bytes

Debugging now, although it's painful because it only appears on windows-opt and doesn't seem to occur when I run any single test, only when I run several tests in a row.
http://hg.mozilla.org/projects/electrolysis/rev/b087c6a13c29

The test wasn't waiting for all its part to complete. I'll file a followup for this case.
So is this good to land on m-c then?
No, the tree has been orange with various semi-random orange's since this landed.
Filed bug 539038 and disabled the malformed-data test with http://hg.mozilla.org/projects/electrolysis/rev/1e74114cf713 to see if things get less orange. I think that the testplugin is double-deleting its streamnotify context pointer for this test in the in-process case (probably correctly!) which is causing memory corruption.
Attachment #418958 - Attachment is obsolete: true
Attachment #418958 - Flags: review?(benjamin)
Duplicate of this bug: 535576
You need to log in before you can comment on or make changes to this bug.