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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

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

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.

Comment 1

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

Can't easily repro this on hulu.

Comment 2

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

Attachment: test case that triggers crash

I'm pretty sure the malformed-data:-URI test is triggering the crash, but I'd like to keep the other tests here too.

Comment 3

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

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.

Comment 4

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

Attachment: new javascript: and data: URI tests

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.

Comment 5

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

Attachment: don't try to do PStreamNotify error cleanup in PluginInstanceParent

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.

Comment 6

[Benjamin Smedberg]

Attachment: Delete the PStreamNotifyParent only if it hasn't already been deleted, rev. 1

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.

Comment 7

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

(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 8

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

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.

Comment 9

[Jim Jeffery not reading bug-mail 1/2/11]

Is this Linux only ? or are other platforms affected ?

Is this ready now for checkin ?  Not been testing OOPP due to crashes.

Comment 10

[Benjamin Smedberg]

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).

Comment 11

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

http://hg.mozilla.org/projects/electrolysis/rev/077bdb4faf06

Did you mean to land this on e10s instead of m-c?

Comment 12

[Benjamin Smedberg]

Yes, m-c was closed and I wanted to get testing. The e10s orange is concerning!

Comment 14

[Daniel Holbert [:dholbert]]

If it helps, see (duplicate) Bug 538250 comment 2 for STR.
That bug also has a reduced testcase: attachment 420417 [details] [diff] [review]

Comment 15

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

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"

Comment 16

[Benjamin Smedberg]

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.

Comment 17

[Benjamin Smedberg]

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.

Comment 18

[u88484]

So is this good to land on m-c then?

Comment 19

[Benjamin Smedberg]

No, the tree has been orange with various semi-random orange's since this landed.

Comment 20

[Benjamin Smedberg]

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.

Comment 21

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

e10s went green (well, non-unknown orange) after http://hg.mozilla.org/projects/electrolysis/rev/1e74114cf713.

Comment 22

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/908f0fa9cc06
http://hg.mozilla.org/mozilla-central/rev/011f63ee17df
http://hg.mozilla.org/mozilla-central/rev/79e9f413ff5a
http://hg.mozilla.org/mozilla-central/rev/47dc7b3a8676
http://hg.mozilla.org/mozilla-central/rev/be2c775bddab