902271 - Crash on b2g with this particular xmlhttprequest

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Martijn Wargers (dead)]

See url testcase, when loading that testcase, you should get to see 'succeed!' after 500ms.

On the b2g OS, this seems to cause a crash, because I get to see a 'Well, this is embarassing' screen to see after a short while in the b2g browser.

This is the cause of the failure in:
http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug431701.html?force=1
And I think also this one:
http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug422537.html?force=1

Comment 1

[Scoobidiver (away)]

Please provide a stack trace (see https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#Getting_crashes_off_the_Device).

Comment 2

[Martijn Wargers (dead)]

mwargers@ubuntu:~/B2G$ adb shell ls -l /data/b2g/mozilla/Crash\ Reports/submitted/ 
gives me:
/data/b2g/mozilla/Crash Reports/submitted/: No such file or directory

mwargers@ubuntu:~/B2G$ adb shell b2g-ps
gives me:
/system/bin/b2g-ps: 36: Syntax error: Bad substitution

From adb logcat:
E/GeckoConsole(   45): [JavaScript Error: "Error: Permission denied to access property 'top'" {file: "app://browser.gaiamobile.org/js/browser.js" line: 554}]
F/libc    (  441): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)
I/Gecko   (   45): 
I/Gecko   (   45): ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
I/Gecko   (   45): 
I/DEBUG   (  406): debuggerd committing suicide to free the zombie!
I/DEBUG   (  457): debuggerd: Nov 16 2012 00:46:34

That's the only I can get out of the b2g emulator. Are there other ways to get stack traces?

Comment 3

[Robert Kaiser]

I could get a crash report for this, on my Peak with a Nightly build of Firefox OS: bp-7c36897e-57bc-4d53-b0f3-63a562130905

Comment 4

[Robert Kaiser]

Suffice to say it's easy to reproduce by reloading.

Comment 5

[Robert Kaiser]

Somewhat different stack on a 1.1 build on my otoro: bp-49ebe465-4cd4-4891-b8ce-48f892130905

Comment 6

[Josh Matthews [:jdm]]

It's likely that this is coming from sending a Document, which creates an nsStorageInputStream which doesn't inherit from nsIIPCSerializableInputStream.

Comment 7

[Martijn Wargers (dead)]

Are stack traces from crashes on the emulator not possible?

Comment 8

[Boris Zbarsky [:bzbarsky]]

If comment 6 is right (which seems plausible), who should own this change to the IPC code?

Comment 9

[Josh Matthews [:jdm]]

Attachment: Make StorageInputStream serializable cross-process.

This builds, but it's totally untested. If someone else feels inspired they can try it out and even try to fix it up. Recommendations for testing it?

Comment 10

[Josh Matthews [:jdm]]

I will.

Comment 11

[Josh Matthews [:jdm]]

I've got an xpcshell test that reproduces this, which is lovely.

Comment 12

[Josh Matthews [:jdm]]

Attachment: Make StorageInputStream serializable cross-process.

Test passes. Now hopefully somebody can tell me whether this is a sensible way to serialize and deserialize the input stream.

Comment 13

[Benjamin Smedberg]

Comment on attachment 801241 [details] [diff] [review]
Make StorageInputStream serializable cross-process.

I'm absolutely sure that I am not the correct reviewer for this. Who reviewed the original serializable networking streams?

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I reworked all of this a while ago so it's probably me.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 801241 [details] [diff] [review]
Make StorageInputStream serializable cross-process.

I suppose this is Ben's baby.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 801241 [details] [diff] [review]
Make StorageInputStream serializable cross-process.

Review of attachment 801241 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC we may be able to ignore the complexity here and simply serialize as a normal StringInputStreamParams. If you hit problems just re-r? me.

Comment 17

[Josh Matthews [:jdm]]

Attachment: Make StorageInputStream serializable cross-process.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 827684 [details] [diff] [review]
Make StorageInputStream serializable cross-process.

Review of attachment 827684 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Thanks.

::: xpcom/io/nsStorageStream.cpp
@@ +55,5 @@
>  nsStorageStream::nsStorageStream()
>      : mSegmentedBuffer(0), mSegmentSize(0), mWriteInProgress(false),
>        mLastSegmentNum(-1), mWriteCursor(0), mSegmentEnd(0), mLogicalLength(0)
>  {
> +    LOG_(("Creating nsStorageStream [%p].\n", this));

Are all these LOG changes just line endings or something?

@@ +532,5 @@
> +    nsCString combined;
> +    int32_t capacity = mStorageStream->mLastSegmentNum;
> +    uint32_t offset = mReadCursor;
> +    for (int32_t i = mSegmentNum; i <= capacity; i++) {
> +        combined.Append(mStorageStream->mSegmentedBuffer->GetSegment(i) + offset);

Hm, you've got some sign fun going on here... I realize we will most likely never have more than INT32_MAX segments here, but we can just make it safe now and never have to worry about it. How about you just do this:

  nsCString combined;
  if (mStorageStream->mLastSegmentNum >= 0) {
    uint32_t capacity = uint32_t(mStorageStream->mLastSegmentNum);
    uint32_t offset = mReadCursor;
    for (uint32_t i = mSegmentNum; i <= capacity; i++) {
      // ...
    }
  }

Comment 19

[Josh Matthews [:jdm]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #18)
> Comment on attachment 827684 [details] [diff] [review]
> Make StorageInputStream serializable cross-process.
> 
> Review of attachment 827684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great! Thanks.
> 
> ::: xpcom/io/nsStorageStream.cpp
> @@ +55,5 @@
> >  nsStorageStream::nsStorageStream()
> >      : mSegmentedBuffer(0), mSegmentSize(0), mWriteInProgress(false),
> >        mLastSegmentNum(-1), mWriteCursor(0), mSegmentEnd(0), mLogicalLength(0)
> >  {
> > +    LOG_(("Creating nsStorageStream [%p].\n", this));
> 
> Are all these LOG changes just line endings or something?

Renaming LOG to LOG_ because of the Chromium headers. Want me to just do #undef LOG instead?

Comment 20

[(no longer active)]

(In reply to comment #19)
> (In reply to ben turner [:bent] (needinfo? encouraged) from comment #18)
> > Comment on attachment 827684 [details] [diff] [review]
> > Make StorageInputStream serializable cross-process.
> > 
> > Review of attachment 827684 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks great! Thanks.
> > 
> > ::: xpcom/io/nsStorageStream.cpp
> > @@ +55,5 @@
> > >  nsStorageStream::nsStorageStream()
> > >      : mSegmentedBuffer(0), mSegmentSize(0), mWriteInProgress(false),
> > >        mLastSegmentNum(-1), mWriteCursor(0), mSegmentEnd(0), mLogicalLength(0)
> > >  {
> > > +    LOG_(("Creating nsStorageStream [%p].\n", this));
> > 
> > Are all these LOG changes just line endings or something?
> 
> Renaming LOG to LOG_ because of the Chromium headers. Want me to just do #undef
> LOG instead?

This should no longer be an issue, I renamed those macros.

Comment 21

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/629aa51ebfaf

Comment 22

[Josh Matthews [:jdm]]

Backed out for ASAN errors on the new test: https://hg.mozilla.org/integration/mozilla-inbound/rev/08e0f1a9d9fa

https://tbpl.mozilla.org/php/getParsedLog.php?id=32540159&tree=Mozilla-Inbound

Comment 24

[Martijn Wargers (dead)]

When fixing this, could you also remove the lines in the b2g.json file that are about this?
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/b2g.json#93

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What is the status of this?

Comment 26

[Josh Matthews [:jdm]]

It bounced with ASAN errors and I haven't looked at it since.

Comment 27

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=f8c62171f162

Trying a new approach.

Comment 28

[Josh Matthews [:jdm]]

Attachment: Make StorageInputStream serializable cross-process

ASAN did not complain about this one, and it involves understanding 100% less of the implementation of nsStorageInputStream and nsSegementedBuffer.

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8428297 [details] [diff] [review]
Make StorageInputStream serializable cross-process

Review of attachment 8428297 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: xpcom/io/nsStorageStream.cpp
@@ +594,5 @@
> +
> +  combined.SetCapacity(remaining);
> +  uint32_t numRead = 0;
> +
> +  rv = Read(combined.BeginWriting(), remaining, &numRead);

This won't close on EOF will it?

Comment 30

[Josh Matthews [:jdm]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #29)
> ::: xpcom/io/nsStorageStream.cpp
> @@ +594,5 @@
> > +
> > +  combined.SetCapacity(remaining);
> > +  uint32_t numRead = 0;
> > +
> > +  rv = Read(combined.BeginWriting(), remaining, &numRead);
> 
> This won't close on EOF will it?

It doesn't appear so from reading the code.

Comment 31

[Josh Matthews [:jdm]]

Attachment: Make StorageInputStream serializable cross-process

Comment 32

[Josh Matthews [:jdm]]

Just waiting for the tree to open before this can land.

Comment 33

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4426be8be94c

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4426be8be94c