1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Andrew McCreight [:mccr8]]

When I run dom/plugins/test/mochitest/test_pluginstream_newstream.html with e10s, I get a crash:

calling NPN_NewStream...return value 0
Assertion failure: mRawPtr != 0 (You can't dereference a NULL nsCOMPtr with operator->().), at /Users/amccreight/mc/obj-dbg.noindex/dist/include/nsCOMPtr.h:734
#01: mozilla::plugins::PluginStreamParent::AnswerNPN_Write(nsCString const&, int*) (PluginStreamParent.cpp:43, in XUL)
#02: mozilla::plugins::PPluginStreamParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginStreamParent.cpp:230, in XUL)
#03: mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginModuleParent.cpp:1410, in XUL)
#04: mozilla::ipc::MessageChannel::DispatchInterruptMessage(IPC::Message const&, unsigned long) (MessageChannel.cpp:1453, in XUL)
#05: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) (MessageChannel.cpp:1302, in XUL)
#06: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (MessageChannel.cpp:1275, in XUL)
#07: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (message_loop.cc:365, in XUL)
#08: MessageLoop::DoWork() (message_loop.cc:459, in XUL)
#09: mozilla::ipc::DoWorkRunnable::Run() (MessagePump.cpp:221, in XUL)
#10: nsThread::ProcessNextEvent(bool, bool*) (nsCOMPtr.h:403, in XUL)

The crash is in PluginStreamParent, but oddly enough the crash looks to be in the content process, because I see the "oops your tab crashed!" message in the browser running the test.

The crash looks to be on this line:

  *written = mInstance->mNPNIface->write(mInstance->mNPP, mStream,
                                         data.Length(),
                                         const_cast<char*>(data.get()));

Comment 1

[Andrew McCreight [:mccr8]]

Another odd thing: this test is only disabled in e10s in debug builds, so maybe it doesn't crash in opt builds?

Comment 2

[Benjamin Smedberg]

PluginStreamParent runs in the content process, because that is the parent side of the content<->plugin bridge.

Can you debug and see which thing is null? mInstance isn't a nsCOMPtr so I'm confused about which thing is asserting.

I cannot explain the debug-only bit.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> PluginStreamParent runs in the content process, because that is the parent
> side of the content<->plugin bridge.
Yeah, Bill explained that to me. I didn't realize that was how bridges are set up, but it makes sense.

> Can you debug and see which thing is null? mInstance isn't a nsCOMPtr so I'm
> confused about which thing is asserting.

I'll do that. My best guess is that there's an assertion somewhere in there that does a dereference and we're just getting an odd stack due to inlining.

Comment 4

[Andrew McCreight [:mccr8]]

I got a stack in the debugger. It looks like the null-deref is on nsPluginStreamToFile::mOutputStream, as the deref is on this line:
  mOutputStream->Write(aBuf, aCount, aWriteCount);

* thread #1: tid = 0x27775, 0x0000000102bab009 XUL`nsPluginStreamToFile::Write(char const*, unsigned int, unsigned int*) [inlined] nsCOMPtr<nsIOutputStream>::operator->() const + 72 at nsCOMPtr.h:733, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000102bab009 XUL`nsPluginStreamToFile::Write(char const*, unsigned int, unsigned int*) [inlined] nsCOMPtr<nsIOutputStream>::operator->() const + 72 at nsCOMPtr.h:733
    frame #1: 0x0000000102baafc1 XUL`nsPluginStreamToFile::Write(this=<unavailable>, aBuf=<unavailable>, aCount=<unavailable>, aWriteCount=<unavailable>) + 81 at nsNPAPIPluginStreamListener.cpp:91
    frame #2: 0x0000000102b8a966 XUL`mozilla::plugins::parent::_write(npp=<unavailable>, pstream=<unavailable>, len=1024, buffer=0x000000010bb1a808) + 182 at nsNPAPIPlugin.cpp:888
    frame #3: 0x0000000102bf2def XUL`mozilla::plugins::PluginStreamParent::AnswerNPN_Write(this=0x0000000112a43240, data=<unavailable>, written=0x00007fff5fbfbd74) + 63 at PluginStreamParent.cpp:43
    frame #4: 0x0000000100ba8ef5 XUL`mozilla::plugins::PPluginStreamParent::OnCallReceived(this=0x0000000112a43240, msg__=<unavailable>, reply__=0x00007fff5fbfbe80) + 1253 at PPluginStreamParent.cpp:230
    frame #5: 0x0000000100b96eff XUL`mozilla::plugins::PPluginModuleParent::OnCallReceived(this=<unavailable>, msg__=0x00007fff5fbfbfc8, reply__=0x00007fff5fbfbe80) + 415 at PPluginModuleParent.cpp:1410
    frame #6: 0x00000001009e007c XUL`mozilla::ipc::MessageChannel::DispatchInterruptMessage(this=0x000000010bb1c868, aMsg=0x00007fff5fbfbfc8, stackDepth=<unavailable>) + 716 at MessageChannel.cpp:1452
    frame #7: 0x00000001009dfb42 XUL`mozilla::ipc::MessageChannel::DispatchMessage(this=0x000000010bb1c868, aMsg=<unavailable>) + 530 at MessageChannel.cpp:1302
    frame #8: 0x00000001009da064 XUL`mozilla::ipc::MessageChannel::OnMaybeDequeueOne(this=0x000000010bb1c868) + 244 at MessageChannel.cpp:1275

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Is this failing?
https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPluginStreamListener.cpp#62

Comment 6

[Andrew McCreight [:mccr8]]

It looks like it is this line that is failing:
https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPluginStreamListener.cpp#58

Comment 7

[Andrew McCreight [:mccr8]]

RemoteOpenFileChild::CreateUnique() isn't implemented, and I guess that's what this would be using?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> RemoteOpenFileChild

Are you sure? I thought that was only used for JAR files.

What platform are you testing on? I could imagine the Mac sandbox could affect this.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #8)
> Are you sure? I thought that was only used for JAR files.
Ah, you are right. This is nsLocalFile.
 
> What platform are you testing on? I could imagine the Mac sandbox could
> affect this.

I'm on OSX.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, that must be it then. Not sure who to redirect this to. Brad, who do you think should look at this Mac sandboxing issue?

Comment 11

[Haik Aftandilian [:haik]]

(In reply to Bill McCloskey (:billm) from comment #10)
> OK, that must be it then. Not sure who to redirect this to. Brad, who do you
> think should look at this Mac sandboxing issue?

I could take a crack at this if it's not needed urgently.

Comment 12

[Haik Aftandilian [:haik]]

I think the file we're trying to create (on OS X 10.11.2 (15C50)) with the CreateUnique call in the nsPluginStreamToFile constructor is 

  ~/Library/Caches/TemporaryItems/testframe

When this fails, mOutputStream is never initialized resulting in the nsPluginStreamToFile::Write crash. 

I haven't got to lowest level file creation call that fails yet. I'm wondering if the current OS X sandbox permits the child creating such a file.

Creating a file in the nsPluginStreamToFile constructor seems like a bad idea.

Comment 13

[Stephen A Pohl [:spohl]]

(In reply to Haik Aftandilian [:haik] from comment #12)
> I think the file we're trying to create (on OS X 10.11.2 (15C50)) with the
> CreateUnique call in the nsPluginStreamToFile constructor is 
> 
>   ~/Library/Caches/TemporaryItems/testframe
> 
> When this fails, mOutputStream is never initialized resulting in the
> nsPluginStreamToFile::Write crash. 
> 
> I haven't got to lowest level file creation call that fails yet. I'm
> wondering if the current OS X sandbox permits the child creating such a file.

There seems to have been a similar problem a while back in bug 1190032. The file that couldn't be created due to sandbox rules was ~/Library/Caches/TemporaryItems/plugtmp. The fix appears to have been a bug-specific (i.e. file-specific) workaround in the sandbox ruleset. Thought I'd throw this out there in case it helps.

Comment 14

[Haik Aftandilian [:haik]]

(In reply to Stephen A Pohl [:spohl] from comment #13)
> There seems to have been a similar problem a while back in bug 1190032.

Thanks. Changing the sandbox ruleset in this case would require allowing files with any name to be created in ~/Library/Caches/TemporaryItems/. "testframe" comes from the iframe name in the test. I was surprised to see the iframe name being used in the Cache filename.

Here's the relevant test HTML.

https://dxr.mozilla.org/mozilla-central/rev/531d1f6d1cde1182e9f7f9dff81a4fc5abc0a601/dom/plugins/test/mochitest/test_pluginstream_newstream.html#18

And the open failure is coming from the system's open call libsystem_kernel.dylib`__open. On a Mac, running "sudo opensnoop -x -n plugin-container" displays the path to files for which the open system call fails for the process named plugin-container.

Comment 15

[Haik Aftandilian [:haik]]

I tested on Ubuntu and found that the test passes and the testframe file is put in /tmp.

Comment 16

[Haik Aftandilian [:haik]]

Discussed with Bob and JCP. With the Windows low integrity sandbox, we have a per-profile temp directory and we plan to take the same approach here. i.e., create a per-profile temporary directory in the parent and pass the path to the child via a preference. That will become the child's temporary directory and it will be given permission to create and write to files there. It would be better to be more restrictive and go through the parent for creation of any temporary files, but considering this approach because it minimizes changes needed to support the older nsNPAPI plugins using this stream functionality.

Comment 17

[Haik Aftandilian [:haik]]

Attachment: MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Modify the Mac sandbox to allow temporary files to be created in a parent-specified temporary directory. This replaces the NS_OS_TEMP_DIR directory.

Review commit: https://reviewboard.mozilla.org/r/34023/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34023/

Comment 18

[Haik Aftandilian [:haik]]

The patch I just posted in comment is 17 is incomplete, but I wanted to make sure I'm going in the right direction and ask a question.

With the changes, the content process is reading a full path from the preferences and attempting to replace NS_OS_TEMP_DIR with that directory. I think I need to create an nsIFile in order to do that, but how does one create an nsIFile from an arbitrary local path. I found nsILocalFile, but it is marked as deprecated.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

https://reviewboard.mozilla.org/r/34023/#review30671

::: dom/ipc/ContentProcess.cpp:21
(Diff revision 1)
> +#include "nsDirectoryServiceDefs.h"

These are common with the XP_WIN things, so move them up.

::: dom/ipc/ContentProcess.cpp:47
(Diff revision 1)
> +  // XXX how to create an nsIFile from a string path?

Example code from another component: (note the NS_ENSURE_SUCCESS thing is deprecated)

// Directory providers must also be accessed on the main thread.
  nsCOMPtr<nsIFile> cacheDir;
  rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_LOCAL_50_DIR,
                              getter_AddRefs(cacheDir));
  if (NS_FAILED(rv)) {
    rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
                                getter_AddRefs(cacheDir));
    if (NS_FAILED(rv)) {
      return rv;
    }
  }
// Get the root directory where to store all the databases.
  nsresult rv = cacheDir->Clone(getter_AddRefs(mStoreDirectory));
  NS_ENSURE_SUCCESS(rv, rv);

  rv = mStoreDirectory->AppendNative(STORE_DIRECTORY);
  NS_ENSURE_SUCCESS(rv, rv);

Comment 20

[Haik Aftandilian [:haik]]

I tried using the above NS_GetSpecialDirectory calls, but they fail due to nsDirectoryService::Get returning NS_ERROR_FAILURE. I don't know why yet, but could it be the child is not expected to need them or at this stage the nsDirectoryService isn't fully setup? Could not find where this is all setup in the code base.

The following did work though.

  nsCOMPtr<nsIFile> tempDir;
  tempDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
  tempDir->InitWithPath(tempDirPath);

Comment 21

[Benjamin Smedberg]

You can't get the profile directory from the content process: the content process doesn't run with a profile and doesn't know where the profile is located. That's why a "parent-specified" file/directory is required.

Comment 22

[Haik Aftandilian [:haik]]

Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/1-2/

Comment 23

[Haik Aftandilian [:haik]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #21)
> You can't get the profile directory from the content process: the content
> process doesn't run with a profile and doesn't know where the profile is
> located. That's why a "parent-specified" file/directory is required.

Thanks.

The new patch posted no longer passes the full path to a profile sub-directory to the child, instead does what the Windows version does by passing just a UUID which is used to build a sub-directory name. But the temp sub-directory is created in NS_OS_TEMP_DIR unlike on Windows.

Passing the full path to a profile sub-dir seemed like a bad idea because it exposes the profile dir to the child and I was worried that if the preference was changed, we might delete important user files when cleaning up the temp dir.

I also noticed on Mac we have $TMPDIR which is the same as 

  ~ $ getconf DARWIN_USER_TEMP_DIR
  /var/folders/46/188kdiwu49d3zkkfgrgsnx2m0000gn/T/
  ~ $ echo $TMPDIR
  /var/folders/46/188kdiwu49d3zkkfgrgsnx2m0000gn/T/

And documented in CONFSTR(3) as

     _CS_DARWIN_USER_TEMP_DIR
             Provides the path to a user's temporary items directory. The
             directory will be created it if does not already exist. This
             directory is created with access permissions of 0700 and
             restricted by the umask(2) of the calling process and is a good
             location for temporary files.

             By default, files in this location may be cleaned (removed) by
             the system if they are not accessed in 3 days.

Given that it can be automatically cleaned up by the OS, it might be a better place for this.

Comment 24

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/34023/#review30671

> These are common with the XP_WIN things, so move them up.

Cleaned up in new version.

Comment 25

[Haik Aftandilian [:haik]]

Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/2-3/

Comment 26

[Bob Owen (:bobowen)]

Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

https://reviewboard.mozilla.org/r/34023/#review32447

Few nits, questions and thoughts.

::: browser/app/profile/firefox.js:1210
(Diff revision 3)
> +// ID (a UUID when set by gecko) that is used as a per profile suffix to a low

It would be a pain, but I wonder if where we have the Mac and Windows code in common we shouldn't be using a term like sandbox writable instead of low integrity, as that only really means something on Windows.

::: dom/ipc/ContentChild.cpp:1391
(Diff revision 3)
> +  // sandboxed temporary directory.

nit: I think this should say something like "sandbox writable temporary directory.".

::: dom/ipc/ContentChild.cpp:1401
(Diff revision 3)
> +  char *tempDirPathCString = ToNewCString(tempDirPath);

nit: Why not just do tempDirPath.get() like the others?

::: dom/ipc/ContentProcess.cpp:40
(Diff revision 3)
>    // sandbox pref level >= 1.

nit: This comment needs updating, in fact it might be best to keeps the checks for each OS separate, for clarity.
It's only partially by design that level 1 for Mac and Windows means we need this special temp directory.
They could be different.

::: dom/ipc/ContentProcess.cpp:56
(Diff revision 3)
> +      NS_WIN_LOW_INTEGRITY_TEMP_BASE,

nit: indentation changed ... if it were me I think I'd set a variable in #ifdefs before the call.

::: security/sandbox/mac/Sandbox.mm:428
(Diff revision 3)
> +  "        (home-subpath appTempDir))\n"

First time I've looked at the Mac rules in detail.
So this new dir lives under the home dir if I'm reading this correctly?

::: toolkit/xre/nsAppRunner.cpp:609
(Diff revision 3)
> +      NS_WIN_LOW_INTEGRITY_TEMP_BASE,

nit: As before.

::: toolkit/xre/nsAppRunner.cpp:643
(Diff revision 3)
>    // and sandbox pref level >= 1.

As before.

::: toolkit/xre/nsAppRunner.cpp:704
(Diff revision 3)
>    // We can't have created a low integrity temp before Vista.

nit: inside #ifdef

Comment 27

[Haik Aftandilian [:haik]]

Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/3-4/

Comment 28

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/34023/#review32447

> It would be a pain, but I wonder if where we have the Mac and Windows code in common we shouldn't be using a term like sandbox writable instead of low integrity, as that only really means something on Windows.

Sounds like a good idea. Changed the comments and some variable names in these files to refer to a sandbox-writable temporary directory. Moved the Suffix pref down after the sandbox level because the comments reference the level.

> nit: I think this should say something like "sandbox writable temporary directory.".

Done.

> nit: Why not just do tempDirPath.get() like the others?

Fixed. I think at the time I was confused about string types.

> nit: This comment needs updating, in fact it might be best to keeps the checks for each OS separate, for clarity.
> It's only partially by design that level 1 for Mac and Windows means we need this special temp directory.
> They could be different.

I see. Moved the checks to Mac-specific and Windows-specific functions.

> nit: indentation changed ... if it were me I think I'd set a variable in #ifdefs before the call.

Fixed.

> nit: As before.

Fixed.

> As before.

Fixed.

Comment 29

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/34023/#review32447

> Sounds like a good idea. Changed the comments and some variable names in these files to refer to a sandbox-writable temporary directory. Moved the Suffix pref down after the sandbox level because the comments reference the level.

The above "Moved the Suffix pref down after the sandbox level because the comments reference the level" refers to the firefox.js file.

Comment 30

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/34023/#review32447

> First time I've looked at the Mac rules in detail.
> So this new dir lives under the home dir if I'm reading this correctly?

Yes, it's within the home directory as a result of NS_OS_TEMP_DIR being within the home directory. The "home-subpath" usage in the read and write rules here could be considered redundant because it acts a filter and only allows reads/writes to appTempDir that are also within the home directory. But I think it's also a good failsafe measure preventing the sandboxed-writable temp dir from ever being outside of the home directory. I gleaned that from http://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf

Comment 31

[Haik Aftandilian [:haik]]

Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/4-5/

Comment 32

[Haik Aftandilian [:haik]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=679b8678c4fd

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b20181a5b50

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b20181a5b50