Crash on b2g with this particular xmlhttprequest

RESOLVED FIXED in mozilla33

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: jdm)

Tracking

({crash, reproducible, testcase})

Trunk
mozilla33
ARM
Gonk (Firefox OS)
crash, reproducible, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2g-crash], crash signature, URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Severity: normal → critical

Comment 1

5 years ago
Please provide a stack trace (see https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#Getting_crashes_off_the_Device).
Keywords: stackwanted
Whiteboard: [b2g-crash]
(Reporter)

Comment 2

5 years ago
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

5 years ago
I could get a crash report for this, on my Peak with a Nightly build of Firefox OS: bp-7c36897e-57bc-4d53-b0f3-63a562130905
Crash Signature: [@ mozilla::ipc::SerializeInputStream(nsIInputStream*, mozilla::ipc::InputStreamParams&) ]
Keywords: stackwanted
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM

Comment 4

5 years ago
Suffice to say it's easy to reproduce by reloading.
Keywords: reproducible

Comment 5

5 years ago
Somewhat different stack on a 1.1 build on my otoro: bp-49ebe465-4cd4-4891-b8ce-48f892130905
Crash Signature: [@ mozilla::ipc::SerializeInputStream(nsIInputStream*, mozilla::ipc::InputStreamParams&) ] → [@ mozilla::ipc::SerializeInputStream(nsIInputStream*, mozilla::ipc::InputStreamParams&) ] [@ mozalloc_abort | abort | mozilla::ipc::SerializeInputStream ]
(Assignee)

Comment 6

5 years ago
It's likely that this is coming from sending a Document, which creates an nsStorageInputStream which doesn't inherit from nsIIPCSerializableInputStream.
(Assignee)

Updated

5 years ago
Component: Networking → XPCOM
(Reporter)

Comment 7

5 years ago
Are stack traces from crashes on the emulator not possible?
If comment 6 is right (which seems plausible), who should own this change to the IPC code?
Flags: needinfo?(josh)
(Assignee)

Comment 9

5 years ago
Created attachment 801136 [details] [diff] [review]
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?
(Assignee)

Updated

5 years ago
Assignee: nobody → josh
(Assignee)

Comment 10

5 years ago
I will.
(Assignee)

Comment 11

5 years ago
I've got an xpcshell test that reproduces this, which is lovely.
Flags: needinfo?(josh)
(Assignee)

Comment 12

5 years ago
Created attachment 801241 [details] [diff] [review]
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.
Attachment #801241 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #801136 - Attachment is obsolete: true

Comment 13

5 years ago
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?
Attachment #801241 - Flags: review?(benjamin)
I reworked all of this a while ago so it's probably me.
(Assignee)

Comment 15

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

I suppose this is Ben's baby.
Attachment #801241 - Flags: review?(bent.mozilla)
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.
Attachment #801241 - Flags: review?(bent.mozilla)
(Assignee)

Comment 17

4 years ago
Created attachment 827684 [details] [diff] [review]
Make StorageInputStream serializable cross-process.
Attachment #827684 - Flags: review?(bent.mozilla)
(Assignee)

Updated

4 years ago
Attachment #801241 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Blocks: 926544
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++) {
      // ...
    }
  }
Attachment #827684 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 19

4 years ago
(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

4 years ago
(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.

Updated

4 years ago
Duplicate of this bug: 956479
(Reporter)

Comment 24

4 years ago
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
What is the status of this?
Flags: needinfo?(josh)
(Assignee)

Comment 26

4 years ago
It bounced with ASAN errors and I haven't looked at it since.
Flags: needinfo?(josh)
(Assignee)

Comment 28

4 years ago
Created attachment 8428297 [details] [diff] [review]
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.
Attachment #8428297 - Flags: review?(bent.mozilla)
(Assignee)

Updated

4 years ago
Attachment #827684 - Attachment is obsolete: true
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?
Attachment #8428297 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 30

4 years ago
(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.
(Assignee)

Comment 31

4 years ago
Created attachment 8440735 [details] [diff] [review]
Make StorageInputStream serializable cross-process
(Assignee)

Updated

4 years ago
Attachment #8428297 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Just waiting for the tree to open before this can land.
(Assignee)

Updated

4 years ago
Flags: needinfo?(josh)
https://hg.mozilla.org/mozilla-central/rev/4426be8be94c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1009039
You need to log in before you can comment on or make changes to this bug.