Closed
Bug 1288997
Opened 8 years ago
Closed 8 years ago
MOZ_CRASH(IPC message size is too large) with PContent::Msg_PBlobConstructor
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox48 | --- | unaffected |
firefox49 | --- | unaffected |
relnote-firefox | --- | 50+ |
firefox50 | + | verified |
firefox51 | + | verified |
firefox52 | --- | verified |
People
(Reporter: n.nethercote, Assigned: baku)
References
Details
(5 keywords)
Crash Data
Attachments
(7 files, 4 obsolete files)
34.84 KB,
patch
|
jld
:
review+
bkelly
:
feedback+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
289 bytes,
text/html
|
Details | |
16.86 KB,
patch
|
gchang
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
13.85 KB,
patch
|
bkelly
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.02 KB,
patch
|
jdm
:
review+
gchang
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.74 KB,
patch
|
jld
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.14 KB,
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
Here's a search of all the crashes in 50.0a1 where we crashed because an IPC message was too big: https://crash-stats.mozilla.com/search/?product=Firefox&version=50.0a1&moz_crash_reason=%3DMOZ_CRASH%28IPC%20message%20size%20is%20too%20large%29&_sort=-date&_facets=moz_crash_reason&_facets=ipc_message_name&_facets=ipc_message_size&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=moz_crash_reason#facet-ipc_message_name I see 74 of them in the past 7 days -- it's the second-most common kind of MOZ_CRASH crash -- and 72 of them involve a "PContent::Msg_PBlobConstructor" message. The search result is faceted on several fields including IPC message size, which shows that the sizes are all 100s or 1000s of MBs, i.e. ridiculously large. jld, is this something you know about?
Flags: needinfo?(jld)
Comment 1•8 years ago
|
||
Andrea's a better person to ask about blobs.
Flags: needinfo?(jld) → needinfo?(amarchesini)
Assignee | ||
Comment 2•8 years ago
|
||
What is happening here is that, recently we landed a code for broadcasting BlobURL to any process. This shows a related problem: for memory blobs we send all the content into a IPC message. This means that a simple |URL.createObjectURL(new Blob([something very big]))| can crash FF. The fix should be to use nsIInputStream and RemoteBlobs for memoryBlob too.
Group: dom-core-security
Flags: needinfo?(amarchesini)
Comment 3•8 years ago
|
||
Is this just an OOM crash? We don't really need to hide this then. There are tons of ways to OOM crash Firefox. :)
Comment 4•8 years ago
|
||
Not exactly an OOM, in that this will break Firefox no matter how much RAM you have: <script> var ab = new ArrayBuffer(256 << 20); var b = new Blob([ab]); document.write(URL.createObjectURL(b)); </script> On Linux and Windows I got a sad-tab as expected, but on Mac it just locked up the content process (and in all cases, changing the 256 to 255 avoided the breakage and allowed the document.write to happen); I don't know what's going on with that yet, but it's a little worrying as far as whether we might be missing more crashes than just this.
Comment 5•8 years ago
|
||
Here's what's going on on Mac: frame #2: 0x0000000103285cde libnss3.dylib`PR_Lock + 14 frame #3: 0x0000000103a67eb1 XUL`mozilla::ipc::MessageChannel::Send(IPC::Message*) + 257 frame #4: 0x0000000103fc6432 XUL`mozilla::dom::PCrashReporterChild::SendAnnotateCrashReport(nsCString const&, nsCString const&) + 498 frame #5: 0x0000000106488f8e XUL`CrashReporter::AnnotateCrashReport(nsACString_internal const&, nsACString_internal const&) + 302 frame #6: 0x0000000103a6f3c1 XUL`mozilla::ipc::ProcessLink::SendMessage(IPC::Message*) + 273 frame #7: 0x0000000103a67ec9 XUL`mozilla::ipc::MessageChannel::Send(IPC::Message*) + 281 Note the reentrancy — the content process tries to send crash annotations to the parent on the same channel that's in the middle of sending the message that's causing the crash. I'm not sure how this ever works (although clearly it sometimes does): http://searchfox.org/mozilla-central/rev/336105a0de49f41a2cc203db7b7ee7266d0d558b/ipc/glue/MessageLink.cpp#164
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8775488 -
Flags: review?(bkelly)
Assignee | ||
Comment 7•8 years ago
|
||
We should talk about this :) How blobs work in e10s, is this: if a memory blob is sent to the parent process, we write the content in 1 single IPC message. If the content is too big, we end up crashing as this bug shows. My approach, in these 2 patches, is to use a 'remote' input stream. This means that, we send a nsIInputStream to the parent process, and there, we create a BlobImplStream object. There are 3 main issues here: 1. slice is not implemented yet. But this is an implementation detail. 2. What about if the BlobImpl is sent back to the content child? We create a remoteBlobImpl in the child process, keeping alive a blob in the parent process (the BlobImplStream), and this reads data from the child process using the SendStream object. We should optimize it. 3. This setup, is extremely similar to what we do when we send a blob from parent process to the content process, and the the content process sends the same blobImpl to the parent process again. Here we are smart enough to identify that the received blob is 'known' and we use the original blob in the parent process. We can use the same approach for point 2 as we do for point 3. But this is a big change and would be nice if we do a bit of cleanup of code here. Having the same approach everywhere. And here the real question for you, bkelly: can we have this SendStream child-parent and parent-child? Keep in mind that we already have something similar in blob-IPC code. If we can do this, we can have a new wonderful setup like this: if a blob is sent from A to B (where A and B are 2 processes), we send a nsIInputStream and a Blob unique ID. If this blob unique ID is found in a per-process hashtable, we use the known blobImpl instead the received nsIInputStream. Otherwise we create a BlobImplStream. I'm happy to describe this setup better in a meeting or on IRC.
Flags: needinfo?(bkelly)
Comment 8•8 years ago
|
||
(In reply to Andrea Marchesini (baku - PTO 1/8 - 12/8) from comment #7) > And here the real question for you, bkelly: can we have this SendStream > child-parent and parent-child? Keep in mind that we already have something > similar in blob-IPC code. If we can do this, we can have a new wonderful > setup like this: Making PSendStream work parent-to-child is bug 1274343. I was hesitant to do that initially because its a bit risky for PBackground actors attached to worker threads. We should be able to add enough asserts to make it safe, though. I don't anticipate allowing the same PSendStream instance to be passed from child-to-parent and then back from parent-to-child, though. The parent would have to create a new PSendStream to send back to the child. Personally, I think this is fine and its not worth the effort of optimizing for this kind of round trip at first.
Flags: needinfo?(bkelly)
Comment 9•8 years ago
|
||
Comment on attachment 8775488 [details] [diff] [review] part 1 - PContentChild vs nsIContentChild Review of attachment 8775488 [details] [diff] [review]: ----------------------------------------------------------------- I'm supportive of this change. I was not aware of the nsIContentChild interface when I did PSendStream initially. I think the patch should be reviewed by an IPC peer, though.
Attachment #8775488 -
Flags: review?(bkelly) → feedback+
Comment 10•8 years ago
|
||
Crash volume for signature 'mozilla::ipc::ProcessLink::SendMessageW': - nightly (version 50): 234 crashes from 2016-06-06. - aurora (version 49): 267 crashes from 2016-06-07. - beta (version 48): 2 crashes from 2016-06-06. - release (version 47): 0 crashes from 2016-05-31. - esr (version 45): 0 crashes from 2016-04-07. Crash volume on the last weeks: W. N-1 W. N-2 W. N-3 W. N-4 W. N-5 W. N-6 W. N-7 - nightly 87 72 1 5 14 12 7 - aurora 8 24 8 12 26 80 82 - beta 0 1 0 0 0 0 1 - release 0 0 0 0 0 0 0 - esr 0 0 0 0 0 0 0 Affected platform: Windows
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Comment 11•8 years ago
|
||
I'm often crashing on 50.0a2 when downloading files from mega.nz.
Updated•8 years ago
|
Group: dom-core-security
Comment 12•8 years ago
|
||
(In reply to Andrea Marchesini (baku - PTO 1/8 - 12/8) from comment #2) > What is happening here is that, recently we landed a code for broadcasting > BlobURL to any process. Which is the bug where we landed this?
Keywords: regression
Comment 13•8 years ago
|
||
Too low volume in 48 to care about this regression.
Updated•8 years ago
|
Comment 14•8 years ago
|
||
It would be great if we could identify the cause of the regression.
Flags: needinfo?(amarchesini)
Comment 15•8 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #12) > (In reply to Andrea Marchesini (baku - PTO 1/8 - 12/8) from comment #2) > > What is happening here is that, recently we landed a code for broadcasting > > BlobURL to any process. > > Which is the bug where we landed this? Bug 1279186.
Updated•8 years ago
|
Blocks: 1279186
status-firefox51:
--- → ?
Comment 16•8 years ago
|
||
Requesting tracking for 50 because it looks like this regression is breaking mega.nz. Removing needinfo since Jed replied in comment 15.
tracking-firefox50:
--- → ?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Marco Castelluccio [:marco] (PTO until August 24/25) from comment #14) > It would be great if we could identify the cause of the regression. Bug 1279186.
Assignee | ||
Updated•8 years ago
|
Attachment #8775488 -
Flags: review?(jld)
Comment 18•8 years ago
|
||
Tracking 51+ for this crash. For 50, the initial facet shows only 3 crashes in the last week - ni on Marco to clarity as to the importance of the site in Comment 16. We can certainly track for 50 if it makes sense.
tracking-firefox51:
--- → +
Updated•8 years ago
|
Flags: needinfo?(mcastelluccio)
Comment 19•8 years ago
|
||
Comment on attachment 8775488 [details] [diff] [review] part 1 - PContentChild vs nsIContentChild Review of attachment 8775488 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. This was a learning experience in some of the code it's touching, but, looks good.
Attachment #8775488 -
Flags: review?(jld) → review+
Hi Baku, one of the patches is r+d but not the other. If we have a fix that is ready for Aurora50, I'd be happy to uplift soon.
Flags: needinfo?(amarchesini)
Comment 21•8 years ago
|
||
Crash volume for signature 'mozilla::ipc::ProcessLink::SendMessageW': - nightly (version 51): 230 crashes from 2016-08-01. - aurora (version 50): 757 crashes from 2016-08-01. - beta (version 49): 44 crashes from 2016-08-02. - release (version 48): 0 crashes from 2016-07-25. - esr (version 45): 0 crashes from 2016-05-02. Crash volume on the last weeks (Week N is from 08-22 to 08-28): W. N-1 W. N-2 W. N-3 - nightly 76 67 63 - aurora 248 301 84 - beta 17 12 3 - release 0 0 0 - esr 0 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly #352 #12 - aurora #1569 #7 - beta #142 - release - esr
Comment 22•8 years ago
|
||
The initial facet is restricted to 50 Nightly, this is why there are just a few crashes (nightly is now 51). For 50 Aurora we have 224 crashes in the last week: https://crash-stats.mozilla.com/search/?product=Firefox&version=50.0a2&moz_crash_reason=%3DMOZ_CRASH%28IPC%20message%20size%20is%20too%20large%29&_sort=-date&_facets=moz_crash_reason&_facets=ipc_message_name&_facets=ipc_message_size&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=moz_crash_reason#crash-reports
Flags: needinfo?(mcastelluccio)
Comment 23•8 years ago
|
||
The crash on 49 beta is a different crash and not the regression caused by bug 1279186 (which landed in 50).
Comment 24•8 years ago
|
||
Crash volume for signature 'mozilla::ipc::ProcessLink::SendMessageW': - nightly (version 51): 240 crashes from 2016-08-01. - aurora (version 50): 783 crashes from 2016-08-01. - beta (version 49): 45 crashes from 2016-08-02. - release (version 48): 0 crashes from 2016-07-25. - esr (version 45): 0 crashes from 2016-05-02. Crash volume on the last weeks (Week N is from 08-22 to 08-28): W. N-1 W. N-2 W. N-3 - nightly 76 67 63 - aurora 248 301 84 - beta 17 12 3 - release 0 0 0 - esr 0 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly #352 #10 - aurora #1544 #7 - beta #132 - release - esr
Comment 25•8 years ago
|
||
Sorry for the bugspam :)
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #20) > Hi Baku, one of the patches is r+d but not the other. If we have a fix that > is ready for Aurora50, I'd be happy to uplift soon. The second patch is not ready yet. I'm working on this.
Flags: needinfo?(amarchesini)
Comment 27•8 years ago
|
||
#7 content crash on Aurora 50.0a2 (mega.nz is #221 in the Alexa global list of top sites: http://www.alexa.com/siteinfo/mega.nz).
Comment 28•8 years ago
|
||
Here's an example that causes a crash once opened.
Comment 29•8 years ago
|
||
We're not seeing crash reports on Linux because on Linux we're not crashing, but hanging with the signature "Abort | mozalloc_abort | NS_DebugBreak | nsDebugImpl::Abort | NS_InvokeByIndex" (e.g. https://crash-stats.mozilla.com/report/index/a37a145e-6cce-40f8-9b5c-583852160830).
Crash Signature: [@ mozilla::ipc::ProcessLink::SendMessageW] → [@ mozilla::ipc::ProcessLink::SendMessageW]
[@ Abort | mozalloc_abort | NS_DebugBreak | nsDebugImpl::Abort | NS_InvokeByIndex ]
Keywords: topcrash,
topcrash-win
Assignee | ||
Comment 30•8 years ago
|
||
Attachment #8775492 -
Attachment is obsolete: true
Attachment #8790093 -
Flags: review?(bkelly)
Comment 31•8 years ago
|
||
Comment on attachment 8790093 [details] [diff] [review] part 2 - SendStream for memory blobs Review of attachment 8790093 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable, but I think the fact this does not support WOULD_BLOCK and PSendStream output is async will cause breakage with large streams. ::: dom/base/File.cpp @@ +1238,5 @@ > + nsCOMPtr<nsIInputStream> clonedStream; > + nsCOMPtr<nsIInputStream> replacementStream; > + > + aRv = NS_CloneInputStream(mInputStream, getter_AddRefs(clonedStream), > + getter_AddRefs(replacementStream)); You should make SlicedInputStream implement nsICloneableInputStream to avoid unnecessary copying here. ::: netwerk/base/SlicedInputStream.cpp @@ +44,5 @@ > + return rv; > + } > + > + if (*aLength + mCurPos > mStart + mLength) { > + *aLength -= XPCOM_MIN(*aLength, (*aLength + mCurPos) - (mStart + mLength)); We should have a gtest that validates the different combinations of slicing possibilities here. @@ +72,5 @@ > + return NS_BASE_STREAM_CLOSED; > + } > + > + char buf[4096]; > + while (mCurPos < mStart + mLength && *aResult < aCount) { I think this should at least try to use nsISeekableInputStream to change the current position. If it doesn't work then fall back to this loop. @@ +76,5 @@ > + while (mCurPos < mStart + mLength && *aResult < aCount) { > + uint32_t bytesRead; > + uint64_t bufCount = XPCOM_MIN(aCount - *aResult, (uint32_t)sizeof(buf)); > + nsresult rv = mInputStream->Read(buf, bufCount, &bytesRead); > + if (NS_WARN_IF(NS_FAILED(rv)) || bytesRead == 0) { It would be nice if this supports nsIAsyncInputStream. It would need to check for WOULD_BLOCK here, etc. And I think you actually need it here since you attach a BlobImplStream to the output of a PSendStream which is an async stream. Also, SlicedInputStream cannot be sent using PSendStream unless it is itself an nsIAsyncInputStream and non-blocking. So if you expect these blobs to get passed back over IPC in the future you might as well implement the interface now. @@ +116,5 @@ > + > +NS_IMETHODIMP > +SlicedInputStream::IsNonBlocking(bool *aNonBlocking) > +{ > + *aNonBlocking = false; This seems wrong. If the base stream is blocking then this stream is blocking. ::: netwerk/base/SlicedInputStream.h @@ +11,5 @@ > + > +namespace mozilla { > +namespace net { > + > +class SlicedInputStream final : public nsIInputStream I feel like this should be put in xpcom in a separate patch reviewed by :froydnj.
Attachment #8790093 -
Flags: review?(bkelly) → review-
Assignee | ||
Comment 32•8 years ago
|
||
Attachment #8790093 -
Attachment is obsolete: true
Attachment #8790647 -
Flags: review?(nfroyd)
Attachment #8790647 -
Flags: feedback?(bkelly)
Comment 33•8 years ago
|
||
Comment on attachment 8790647 [details] [diff] [review] part 2 - SendStream for memory blobs Review of attachment 8790647 [details] [diff] [review]: ----------------------------------------------------------------- The commit message for this patch is incorrect. Comments on SlicedInputStream below; I will let bkelly comment on the DOM pieces. I realize the requested number of tests is rather large, but it'd be nice to have a large amount of confidence in this code. ::: dom/base/File.cpp @@ +1245,5 @@ > + return; > + } > + > + if (replacementStream) { > + mInputStream = replacementStream; Might as well use replacementStream.forget() here. ::: netwerk/base/SlicedInputStream.cpp @@ +48,5 @@ > + return rv; > + } > + > + if (*aLength + mCurPos > mStart + mLength) { > + *aLength -= XPCOM_MIN(*aLength, (*aLength + mCurPos) - (mStart + mLength)); This calculation doesn't seem correct. if *aLength + mCurPos is more than mStart + mLength, but still very close to it, you'll make *aLength the amount that *aLength + mCurPos exceeds mStart + mLength by, rather than capping it to something like (mStart + mLength) - mCurPos. @@ +105,5 @@ > + > + uint64_t index = 0; > + if (mCurPos < mStart) { > + index += XPCOM_MIN(mStart - mCurPos, bufCount); > + } It seems like the loop would be more understandable if you enforced the precondition that mCurPos >= mStart outside the loop, as you've done for the seekable stream case above. Might result in a little more code, but ideally you could factor out the reading code into a separate function, to be called from the precondition enforcing code and from the loop itself. @@ +114,5 @@ > + if (mCurPos > mStart + mLength) { > + offset -= XPCOM_MIN((uint64_t)0, mStart + mLength - mCurPos); > + } > + > + if (index == bufCount || offset == 0 || offset - index == 0) { MOZ_ASSERT that index < offset after this condition? It obviously won't affect the loop below, but it would point out a bug in how we were calculating these. @@ +167,5 @@ > + return rv; > + } > + > + if (replacementStream) { > + mInputStream = replacementStream; Might as well use replacementStream.forget() here. ::: netwerk/base/SlicedInputStream.h @@ +13,5 @@ > + > +namespace mozilla { > +namespace net { > + > +class SlicedInputStream final : public nsIAsyncInputStream Please add some kind of documentation for this. Like bkelly said, please move SlicedInputStream + its tests to XPCOM. ::: netwerk/test/gtest/TestSlicedInputStream.cpp @@ +15,5 @@ > + > +public: > + NS_DECL_THREADSAFE_ISUPPORTS > + > + SimpleInputStream(char* aBuf, uint32_t aLength) Does NS_NewByteInputStream or NS_NewCStringInputStream not work for you? I see that you use them below, but you also use this class. Ah, the distinction is that string streams are seekable, and these are not. Perhaps a better name would be NonSeekableStringStream, then, with a comment saying something like: "We want to ensure that sliced streams work with both seekable and non-seekable input streams. As our string streams are seekable, we need to provide a string stream that doesn't permit seeking, so we can test the logic that emulates seeking in sliced input streams." Alternatively, maybe you could get away with a class that just wraps an nsICOMPtr<nsIInputStream> member that *is* a string stream, use NS_FORWARD_NSIINPUTSTREAM to the string stream, and then the QI implementation for the class would be as you have now? @@ +96,5 @@ > + RefPtr<mozilla::net::SlicedInputStream> sis = > + new mozilla::net::SlicedInputStream(stream, 0, BUF_SIZE); > + > + uint64_t length; > + ASSERT_EQ(NS_OK, sis->Available(&length)); There should be some tests for Available() in the middle of reads (haven't started reading the slice, about to start reading the slice, in the middle of the slice, close to the end of the slice, at the end of the slice, etc.). @@ +102,5 @@ > + > + char buf2[BUF_SIZE]; > + uint32_t count; > + ASSERT_EQ(NS_OK, sis->Read(buf2, sizeof(buf2), &count)); > + ASSERT_TRUE(nsCString(buf).Equals(nsCString(buf2))); Using nsDependentCString in these sorts of asserts would make the code ever so slightly faster. @@ +119,5 @@ > + NS_ASSIGNMENT_DEPEND)); > + ASSERT_TRUE(!!stream); > + > + RefPtr<mozilla::net::SlicedInputStream> sis = > + new mozilla::net::SlicedInputStream(stream, 10, 100); We should also have tests where the requested slice is actually outside the bounds of the wrapped stream--i.e. something like 4000, 5000 for this particular example. @@ +127,5 @@ > + ASSERT_EQ((uint64_t)100, length); > + > + char buf2[BUF_SIZE / 2]; > + uint32_t count; > + ASSERT_EQ(NS_OK, sis->Read(buf2, sizeof(buf2), &count)); We should also test reads after the slice has been exhausted. Testing multiple reads from a slice would be good too, rather than "hey, let's just read the slice in one go." @@ +132,5 @@ > + ASSERT_EQ((uint64_t)100, count); > + ASSERT_TRUE(nsCString(buf + 10, count).Equals(nsCString(buf2, count))); > +} > + > +TEST(TestSlicedInputStream, SlicedNoSeek) { It's a bit painful for the tests to have so much copy-and-paste code; the best way I can think of is to write: void BasicSlicedTest(mozilla::function<already_AddRefed<nsIInputStream>(const char*, size_t)> aStreamCreator) { ... nsCOMPtr<nsIInputStream> stream = aStreamCreator(buf, sizeof(buf)); RefPtr<SlicedInputStream> sis = new SlicedInputStream(...); ... } (or template it over the type of aStreamCreator, if you like that approach better.) Not sure if that's overkill for these tests or not. I think it'd be less overkill if we started adding some of the tests above, though.
Attachment #8790647 -
Flags: review?(nfroyd)
Assignee | ||
Comment 34•8 years ago
|
||
> > + if (*aLength + mCurPos > mStart + mLength) {
> > + *aLength -= XPCOM_MIN(*aLength, (*aLength + mCurPos) - (mStart + mLength));
>
> This calculation doesn't seem correct. if *aLength + mCurPos is more than
> mStart + mLength, but still very close to it, you'll make *aLength the
> amount that *aLength + mCurPos exceeds mStart + mLength by, rather than
> capping it to something like (mStart + mLength) - mCurPos.
If mCurPos + *aLength is > than mStart * mLength it means that we have more than what we want.
Here I trim the end of the buffer: I want to remove everything that exceeds the mCurPos + *aLength - mStart + mLength.
To be more precise:
Buffer: [aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]
Sliced: [bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb]<-->
Current: (doesn't matter where mCurPos is) ^ mCurPos + *aLength
In this case I want to reduce *aLength of the difference between mCurPos + *aLength and the max we have to return (mStart + mlength);
Assignee | ||
Comment 35•8 years ago
|
||
If you prefer I can split the patch in 2: SlicedInputStream and the DOM part.
Attachment #8790647 -
Attachment is obsolete: true
Attachment #8790647 -
Flags: feedback?(bkelly)
Attachment #8790886 -
Flags: review?(nfroyd)
Comment 36•8 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #34) > > > + if (*aLength + mCurPos > mStart + mLength) { > > > + *aLength -= XPCOM_MIN(*aLength, (*aLength + mCurPos) - (mStart + mLength)); > > > > This calculation doesn't seem correct. if *aLength + mCurPos is more than > > mStart + mLength, but still very close to it, you'll make *aLength the > > amount that *aLength + mCurPos exceeds mStart + mLength by, rather than > > capping it to something like (mStart + mLength) - mCurPos. > > If mCurPos + *aLength is > than mStart * mLength it means that we have more > than what we want. > Here I trim the end of the buffer: I want to remove everything that exceeds > the mCurPos + *aLength - mStart + mLength. > To be more precise: > > Buffer: [aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] > Sliced: [bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb]<--> > Current: (doesn't matter where mCurPos is) ^ mCurPos + *aLength > > In this case I want to reduce *aLength of the difference between mCurPos + > *aLength and the max we have to return (mStart + mlength); Indeed, my mistake. I think I was assuming it was straight-up assignment and not subtract-assignment.
Comment 37•8 years ago
|
||
Comment on attachment 8790886 [details] [diff] [review] part 2 - SendStream for memory blobs Review of attachment 8790886 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments below. I think splitting the patch into two pieces (xpcom/ and dom/) is reasonable. You still need to fix the commit message for the patch. ::: xpcom/io/SlicedInputStream.cpp @@ +40,5 @@ > + return NS_BASE_STREAM_CLOSED; > + } > + > + nsresult rv = mInputStream->Available(aLength); > + if (NS_WARN_IF(NS_FAILED(rv))) { Does this do the right thing if the underlying stream is asynchronous and doesn't currently have any data? ::: xpcom/io/SlicedInputStream.h @@ +10,5 @@ > +#include "nsCOMPtr.h" > +#include "nsIAsyncInputStream.h" > +#include "nsICloneableInputStream.h" > + > +// This inputStream is able to slice the size of another inputStream. I think this comment makes sense in the context of Blobs, but I'm not sure it makes sense as its own standalone thing. Maybe: "A wrapper for a slice of an underlying input stream." @@ +21,5 @@ > + NS_DECL_NSIINPUTSTREAM > + NS_DECL_NSICLONEABLEINPUTSTREAM > + NS_DECL_NSIASYNCINPUTSTREAM > + > + SlicedInputStream(nsIInputStream* aInputStream, Something informative about how to use this would be nice. "Create an input stream whose data comes from a slice of aInputStream. The slice begins at aStart bytes beyond aInputStream's current position, and extends for a maximum of aLength bytes. If aInputStream contains fewer than aStart bytes, reading from SlicedInputStream returns no data. If aInputStream contains more than aStart bytes, but fewer than aStart + aLength bytes, reading from SlicedInputStream returns as many bytes as can be consumed from aInputStream after reading aLength bytes. aInputStream should not be read from after constructing a SlicedInputStream wrapper around it." ::: xpcom/tests/gtest/TestSlicedInputStream.cpp @@ +92,5 @@ > +} > + > +// Same start, same length. > +TEST(TestSlicedInputStream, Simple) { > + #define BUF_SIZE 4096 Rather than #define/#undef clutter, you could just: const size_t kBufSize = 4096; here and everywhere below, so you could at least eliminate the #undefs.
Attachment #8790886 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 38•8 years ago
|
||
Assignee | ||
Comment 39•8 years ago
|
||
Attachment #8790886 -
Attachment is obsolete: true
Attachment #8791495 -
Flags: review?(bkelly)
Comment 40•8 years ago
|
||
Comment on attachment 8791495 [details] [diff] [review] part 3 - SendStream for memory blobs Review of attachment 8791495 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/File.cpp @@ +1238,5 @@ > + nsCOMPtr<nsIInputStream> clonedStream; > + nsCOMPtr<nsIInputStream> replacementStream; > + > + aRv = NS_CloneInputStream(mInputStream, getter_AddRefs(clonedStream), > + getter_AddRefs(replacementStream)); Can the two sides of the clone be read at different rates? Specifically, is it possible for one side to remain open and never read? If yes, then we probably need to fix bug 1134372 in order for you to use this here. ::: dom/ipc/Blob.cpp @@ +955,5 @@ > MOZ_ALWAYS_TRUE(!rv.Failed()); > > + AutoIPCStream autoStream; > + autoStream.Serialize(inputStream, aManager); > + aBlobData = autoStream.TakeValue(); This kind of defeats the purpose of the RAII class here. If any failures occur after returning from this method and before actually sending it across IPC then you are going to leak stuff. It would be slightly nicer to pass in the AutoIPCStream or your own AutoBlobData wrapper RAII object. If you can guarantee there are no error paths after this, though, I guess its ok. Please add a comment indicating that leaks will occur if the BlobData is not actually sent.
Attachment #8791495 -
Flags: review?(bkelly) → review+
Added to Fx50 Beta release notes.
relnote-firefox:
--- → 50+
Comment 42•8 years ago
|
||
I've reprocessed my crash (https://crash-stats.mozilla.com/report/index/a37a145e-6cce-40f8-9b5c-583852160830) and it got a different signature now: AsyncShutdownTimeout | quit-application-granted | SessionStore: flushing all windows.
Crash Signature: [@ mozilla::ipc::ProcessLink::SendMessageW]
[@ Abort | mozalloc_abort | NS_DebugBreak | nsDebugImpl::Abort | NS_InvokeByIndex ] → [@ mozilla::ipc::ProcessLink::SendMessageW]
[@ Abort | mozalloc_abort | NS_DebugBreak | nsDebugImpl::Abort | NS_InvokeByIndex ]
[@ AsyncShutdownTimeout | quit-application-granted | SessionStore: flushing all windows ]
Comment 43•8 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/11eb7909e663 memory blob should not be shared across processes - part 1 -PSendStream should use nsIContentChild, r=jld, f=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa97d674377 memory blob should not be shared across processes - part 2 - SlicedInputStream, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/f822594d9e9a memory blob should not be shared across processes - part 3 - SendStream for memory blobs, r=bkelly
Comment 44•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11eb7909e663 https://hg.mozilla.org/mozilla-central/rev/7fa97d674377 https://hg.mozilla.org/mozilla-central/rev/f822594d9e9a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 45•8 years ago
|
||
I'm still experiencing the hang on Linux in the latest Nightly, I haven't tested on Windows. I see the following message in the shell: ###!!! [Parent][MessageChannel] Error: (msgtype=0xF4000C,name=PStorage::Msg_Observe) Channel error: cannot send/recv
Assignee | ||
Comment 46•8 years ago
|
||
> ###!!! [Parent][MessageChannel] Error:
> (msgtype=0xF4000C,name=PStorage::Msg_Observe) Channel error: cannot send/recv
PStorage, it seems unrelated. Can you post a bigger backtrace?
Comment 47•8 years ago
|
||
https://crash-stats.mozilla.com/report/index/b89c104a-26cb-4de9-8bc6-4cee02160923
Comment 48•8 years ago
|
||
Andrea, aren't you able to reproduce it with the 'crasher.html' attachment? I think I'm not the only one able to reproduce, see bug 1255977 comment 20.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 49•8 years ago
|
||
Right, Bug 1294450 will fix this crash.
Flags: needinfo?(amarchesini)
Hi Marco, Baku, if the fix here fixes some of the crashes (even if not all), it's worthwhile uplifting to Beta50 I think. Should we uplift to Aurora and Beta?
Flags: needinfo?(mcastelluccio)
Flags: needinfo?(amarchesini)
Comment 51•8 years ago
|
||
I haven't tested on Windows, which is where the crash is happening, perhaps Andrei or Florin can help? You just need to open the attachment "crasher.html" (or, download a file bigger than 256 MB from mega.nz). If Firefox doesn't crash, then the bug is fixed.
Flags: qe-verify+
Flags: needinfo?(mcastelluccio)
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Comment 52•8 years ago
|
||
Adding a link to a 300 MB anime video file for QA testing purposes & extra verification: https://mega.nz/#!ZpklkLxR!V8e5b9DlzktBL5Mi-AZ1XuXTCq-LnPiTV6hfu3dUGVQ (anime: Akame ga Kill!)
Assignee | ||
Comment 53•8 years ago
|
||
Comment on attachment 8791494 [details] [diff] [review] part 2 - SlicedInputStream Approval Request Comment [Feature/regressing bug #]: Blob and multi-e10s [User impact if declined]: a crash can happen if the blob size is bigger than the max size of a IPC message [Describe test coverage new/current, TreeHerder]: green on try. [Risks and why]: We have 3 patches here. Complex, somehow. But they are fully green and they fix a top crash. [String/UUID change made/needed]: none Note: all the 3 patches must be uplifted.
Flags: needinfo?(amarchesini)
Attachment #8791494 -
Flags: approval-mozilla-beta?
Attachment #8791494 -
Flags: approval-mozilla-aurora?
Comment 54•8 years ago
|
||
I've confirmed with bisect that this also has the side effect of plugging half of a leak similar to bug 1272078 I've been experiencing with the Tab Center addon from Test Pilot (so maybe it plugs bug 1272078 too). (The other half of the leak /might/ be due to Tab Center itself, I have yet to identify what's keeping the objects around, but closing tabs releases them now, which wasn't happening before this bug)
Comment 55•8 years ago
|
||
I can still reproduce the crash on Windows: https://crash-stats.mozilla.com/report/index/ccaa90a9-c89b-4ffe-8944-f8a2a2160929.
Status: RESOLVED → REOPENED
Flags: needinfo?(amarchesini)
Resolution: FIXED → ---
Comment 56•8 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #53) > [Feature/regressing bug #]: Blob and multi-e10s To be precise, this would be bug 1279186, according to comment 15 and comment 17. > [Describe test coverage new/current, TreeHerder]: green on try. Would it be possible to add the 'crasher.html' attachment as a testcase?
Comment 57•8 years ago
|
||
I've managed to reproduce this issue on Windows 10 x64 and Windows 7 x64 with steps provided by Marco, in comment 51. I can confirm that Firefox crashed on all affected builds 50.0b2 (2016-09-26), 51.0a2 (2016-09-29) and 52.0a1 (2016-09-28): https://crash-stats.mozilla.com/report/index/81845381-0c2c-4a2b-b89f-cfc2f2160929 When I disabled e10s, Firefox did not crashed anymore. (this happened with all affected versions) If I can help with more info, please let me know.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Updated•8 years ago
|
Summary: MOZ_CRASH(IPC message size is too large) → MOZ_CRASH(IPC message size is too large) with PContent::Msg_PBlobConstructor
Comment on attachment 8791494 [details] [diff] [review] part 2 - SlicedInputStream It seems based on our testing of the fix on Nightly52, this fix doesn't really fix the crash at all. Is that right? In that case, should we be really uplifting this fix? Hi Baku, Marco, I am reiterating my suggestion, if this fix reduces the occurrences of the crash it is worth uplifting. If it doesn't fix a single occurrence of this crash, then we are better off spinning another fix. Please let me know if I am misunderstand the situation.
Flags: needinfo?(mcastelluccio)
Attachment #8791494 -
Flags: approval-mozilla-beta?
Attachment #8791494 -
Flags: approval-mozilla-beta-
Attachment #8791494 -
Flags: approval-mozilla-aurora?
Attachment #8791494 -
Flags: approval-mozilla-aurora-
Assignee | ||
Comment 59•8 years ago
|
||
In order to fix this crash we must fix also bug 1294450. Bug 1294450 is in m-i, so, next nightly should work fine.
Flags: needinfo?(amarchesini)
Updated•8 years ago
|
Flags: needinfo?(mcastelluccio)
Assignee | ||
Comment 60•8 years ago
|
||
Sorry for asking this, but, Marco, can you verify this again tomorrow?
Flags: needinfo?(mcastelluccio)
Comment 61•8 years ago
|
||
It's still crashing, although with a different signature and intermittently. https://crash-stats.mozilla.com/report/index/36d4ac2f-5745-4cc0-8a96-4644b2161001 https://crash-stats.mozilla.com/report/index/3a3e7e37-d20d-4cb6-864c-4f7502161001 https://crash-stats.mozilla.com/report/index/1021953d-9148-4b50-8100-8f8f72161001 https://crash-stats.mozilla.com/report/index/ebf835fe-1194-40e6-bdcb-170392161001 (these happened directly when downloading the file from mega.nz or opening crasher.html) https://crash-stats.mozilla.com/report/index/b5f4fd08-7569-4184-ab42-f5f422161001 (this happened soon after restaring the browser) I'm needinfoing jdm as well, since he wrote the patch in bug 1294450.
Flags: needinfo?(mcastelluccio)
Flags: needinfo?(josh)
Flags: needinfo?(amarchesini)
Comment 62•8 years ago
|
||
The code that is crashing is: case type__::TPSendStreamChild: { PSendStreamParent* tmp = nullptr; (*(v__)) = tmp; if ((!(Read((&((v__)->get_PSendStreamParent())), msg__, iter__, false)))) { FatalError("Error deserializing Union type"); return false; }
Comment 63•8 years ago
|
||
Sorry, not clear that I can be any more helpful than filing bug 1306983 at this point.
Flags: needinfo?(josh)
Comment 64•8 years ago
|
||
Andrea, correct me if I'm wrong. In order to fix this crash, we need to uplift your patches from this bug, jdm's patches from bug 1294450 and the patches that are going to fix bug 1306979. Is backing out bug 1279186 from beta a simpler/safer option?
Assignee | ||
Comment 65•8 years ago
|
||
This patch is almost like disabling the blobURL support for multi-e10s. In the meantime we can debug why nsIInputStreams have some issues with IPC.
Flags: needinfo?(amarchesini)
Attachment #8797122 -
Flags: review?(josh)
Updated•8 years ago
|
Attachment #8797122 -
Flags: review?(josh) → review+
Assignee | ||
Comment 66•8 years ago
|
||
This is racy (but quite easy to reproduce): The reason why we crash is because AutoIPCStream, in the DTOR, starts pulling data from the nsIInputStream. We are here: SendStreamChildImpl::DoRead(); Now, the |while(true) { ... OnEnd()}| could be faster than the SendPBlobConstructor(). At this point we call SendClose() before creating the PBlob in the parent process. We unregister the PSendStream and when the RecvPBlobConstructor is executed, the PSendStream doesn't exist anymore and we crash. In this patch I postpone the DTOR of AutoIPCStream.
Attachment #8797156 -
Flags: review?(bkelly)
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 67•8 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/44d130271ebc We should not broadcast Blob URLs if we have only 1 content process, r=jdm
Comment 68•8 years ago
|
||
(In reply to Pulsebot from comment #67) > Pushed by amarchesini@mozilla.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/44d130271ebc > We should not broadcast Blob URLs if we have only 1 content process, r=jdm Does this mean that blob URLs registered in the parent process can't be accessed from the content process if multi-e10s isn't enabled? If so, this is a regression: that use case is how add-on authors can be sandbox-friendly while using local files with components like nsIStyleSheetService that broadcast URIs.
Comment 69•8 years ago
|
||
(In reply to Jed Davis [:jld] {⏰UTC-6} from comment #68) > (In reply to Pulsebot from comment #67) > > Pushed by amarchesini@mozilla.com: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/44d130271ebc > > We should not broadcast Blob URLs if we have only 1 content process, r=jdm > > Does this mean that blob URLs registered in the parent process can't be > accessed from the content process if multi-e10s isn't enabled? If so, this > is a regression: that use case is how add-on authors can be sandbox-friendly > while using local files with components like nsIStyleSheetService that > broadcast URIs. No; the code directly above the newly added code checks for the parent process and follows a different code path.
Comment 70•8 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #66) > Created attachment 8797156 [details] [diff] [review] > Change the use of AutoIPCStream > > This is racy (but quite easy to reproduce): The reason why we crash is > because AutoIPCStream, in the DTOR, starts pulling data from the > nsIInputStream. > We are here: SendStreamChildImpl::DoRead(); > > Now, the |while(true) { ... OnEnd()}| could be faster than the > SendPBlobConstructor(). At this point we call SendClose() before creating > the PBlob in the parent process. > > We unregister the PSendStream and when the RecvPBlobConstructor is executed, > the PSendStream doesn't exist anymore and we crash. > > In this patch I postpone the DTOR of AutoIPCStream. bkelly's away this week; it probably makes sense to find another reviewer. Maybe whoever originally reviewed the implementation of PSendStream, if that makes sense?
The patch in comment 66 fixes the problem in bug 1306979 comment 6 for me. We should get this reviewed since it's a major crash.
Comment on attachment 8797156 [details] [diff] [review] Change the use of AutoIPCStream Jed, can you review this since Ben is out?
Attachment #8797156 -
Flags: review?(bkelly) → review?(jld)
Comment 73•8 years ago
|
||
Comment on attachment 8797156 [details] [diff] [review] Change the use of AutoIPCStream Review of attachment 8797156 [details] [diff] [review]: ----------------------------------------------------------------- I needed to spend a little time remembering how AutoIPCStream's lifetimes are supposed to work, but I follow what's going on here. r=me. (It'd be nice if we had region types to help avoid use-after-destroy bugs like this....)
Attachment #8797156 -
Flags: review?(jld) → review+
Comment 74•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44d130271ebc
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 75•8 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/18cb108e1c62 AutoIPCStream DTOR must run after the use of the inputStream for PBlob, r=jld
Assignee | ||
Comment 76•8 years ago
|
||
Marco, can you please test it again when these 2 patches land on m-c? In particular I would like to have feedbacks about this second patch - locally it works fine :) Thanks!
Assignee | ||
Comment 77•8 years ago
|
||
Here your crashtest added.
Attachment #8797401 -
Flags: review?(mcastelluccio)
Comment 78•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/18cb108e1c62
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 79•8 years ago
|
||
Comment on attachment 8797401 [details] [diff] [review] crashtest.patch Review of attachment 8797401 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! (In reply to Andrea Marchesini [:baku] from comment #76) > Marco, can you please test it again when these 2 patches land on m-c? In > particular I would like to have feedbacks about this second patch - locally > it works fine :) > Thanks! I've tested the new Nightly and it works for me as well.
Attachment #8797401 -
Flags: review?(mcastelluccio) → review+
Comment 80•8 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c24253346eac Crashtest added, r=marco
Comment 82•8 years ago
|
||
Backed out for crashtest failures, https://treeherder.mozilla.org/logviewer.html#?job_id=37148830&repo=mozilla-inbound#L2850
Comment 83•8 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd87fff79b4a Backed out changeset c24253346eac for crashtest failures
Comment 85•8 years ago
|
||
Andrea, what do you suggest doing for Beta and Aurora? Could you list the patches, from this bug and other bugs, that would need to be uplifted in order to fix the bug?
Flags: needinfo?(amarchesini)
Updated•8 years ago
|
Crash Signature: [@ mozilla::ipc::ProcessLink::SendMessageW]
[@ Abort | mozalloc_abort | NS_DebugBreak | nsDebugImpl::Abort | NS_InvokeByIndex ]
[@ AsyncShutdownTimeout | quit-application-granted | SessionStore: flushing all windows ] → [@ mozilla::ipc::ProcessLink::SendMessageW]
[@ Abort | mozalloc_abort | NS_DebugBreak | nsDebugImpl::Abort | NS_InvokeByIndex ]
[@ AsyncShutdownTimeout | quit-application-granted | SessionStore: flushing all windows ]
[@ OOM | large | mozalloc_abort | …
Assignee | ||
Comment 86•8 years ago
|
||
Comment on attachment 8775488 [details] [diff] [review] part 1 - PContentChild vs nsIContentChild Approval Request Comment [Feature/regressing bug #]: bug 1279186 [User impact if declined]: a crash when a big memory blob is used to create a blob URL. [Describe test coverage new/current, TreeHerder]: tested, but no test is added because the crasher.html cannot be added as crashtest [Risks and why]: there are many patches but well tested. [String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8775488 -
Flags: approval-mozilla-beta?
Attachment #8775488 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 87•8 years ago
|
||
Comment on attachment 8791494 [details] [diff] [review] part 2 - SlicedInputStream Also this patch must be uplifted to aurora/beta
Assignee | ||
Updated•8 years ago
|
Attachment #8791495 -
Flags: approval-mozilla-beta?
Attachment #8791495 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8797122 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•8 years ago
|
Attachment #8797156 -
Flags: approval-mozilla-beta?
Attachment #8797156 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 88•8 years ago
|
||
So, basically, for beta we can just uplift 8797122. Aurora all the rest.
(In reply to Andrea Marchesini [:baku] from comment #88) > So, basically, for beta we can just uplift 8797122. > Aurora all the rest. I would prefer to uplift only that which is needed and is verified to help. Will clear beta:? noms on all patches except 8797122.
Attachment #8775488 -
Flags: approval-mozilla-beta?
Attachment #8791495 -
Flags: approval-mozilla-beta?
Attachment #8797156 -
Flags: approval-mozilla-beta?
Comment on attachment 8797122 [details] [diff] [review] Disable broadcasting Blob URL if we have only 1 content process As Baku suggested, let's uplift only this patch to Beta50.
Attachment #8797122 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/70abfe99097824fd510544b188f24c588fd6d5a0 I'm not quite sure which patches need uplifted to aurora. Could you clarify?
Flags: needinfo?(amarchesini)
Updated•8 years ago
|
Assignee | ||
Comment 92•8 years ago
|
||
Here the list of patches from m-c: https://hg.mozilla.org/mozilla-central/rev/11eb7909e663 https://hg.mozilla.org/mozilla-central/rev/7fa97d674377 https://hg.mozilla.org/mozilla-central/rev/f822594d9e9a https://hg.mozilla.org/mozilla-central/rev/44d130271ebc https://hg.mozilla.org/mozilla-central/rev/18cb108e1c62
Flags: needinfo?(amarchesini)
Comment 93•8 years ago
|
||
Hi :baku, Can you create the uplift requests for 51 aurora?
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Attachment #8797122 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Crash Signature: | nsCOMPtr_base::assign_from_qi | nsTArray_Impl<T>::InsertElementsAt<T> | nsTArray_Impl<T>::SetLength<T> | mozilla::dom::`anonymous namespace''::BlobDataFromBlobImpl] → | nsCOMPtr_base::assign_from_qi | nsTArray_Impl<T>::InsertElementsAt<T> | nsTArray_Impl<T>::SetLength<T> | mozilla::dom::`anonymous namespace''::BlobDataFromBlobImpl]
[@ mozilla::ipc::ProcessLink::SendMessageW | IPC_Message_Name=PContent::Msg_PBlobConst…
Comment 94•8 years ago
|
||
Comment on attachment 8775488 [details] [diff] [review] part 1 - PContentChild vs nsIContentChild Fix a crash and take suggestion from comment #88. Take it in 51 aurora.
Attachment #8775488 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8791494 -
Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8791495 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8797122 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8797156 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 95•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f35e13d63e28 https://hg.mozilla.org/releases/mozilla-aurora/rev/d388cbcb8213 https://hg.mozilla.org/releases/mozilla-aurora/rev/30a60c96657c https://hg.mozilla.org/releases/mozilla-aurora/rev/4dc5626ae64f https://hg.mozilla.org/releases/mozilla-aurora/rev/09b4ee752f17
Comment 96•8 years ago
|
||
I verified this on latest Nightly 52.0a1 (Build ID 20161109030210) using the instructions from comment 51 and comment 52 and no crash occurred during my testing. I verified on: * Windows 10 x64 - Nightly x64 build and x32 build * Windows 8.1 x64 - Nightly x64 build and x32 build * Windows 7 x32 - Nightly x32 build * Ubuntu 16.04 x64 - Nightly x64 * Mac 10.11 - Nightly x64
Comment 97•7 years ago
|
||
Reproduced the crash with Nightly 2016-09-21 using the links from comment 28 and comment 52. The crashes no longer occur using Firefox 51 beta 2 under Win 10 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.11.
The test originally landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/c24253346eac appears to have been backed out and never relanded. Should it be?
Flags: needinfo?(amarchesini)
Comment 99•7 years ago
|
||
I am still having this problem with the latest release (53.0): mozilla::ipc::ProcessLink::SendMessage | IPC_Message_Name=PBackground::Msg_PBlobConstructor MOZ_CRASH(IPC message size is too large) OS X 10.12 10.12.4 16E195 amd64 It says "verified fixed" at the top, but it does not seem to be actually fixed in the current release?
Comment 100•7 years ago
|
||
Can you file a new bug, explaining how you can reproduce the problem?
Flags: needinfo?(keean)
Assignee | ||
Comment 101•7 years ago
|
||
This bug is already gone in nightly because of the pblob refactoring: bug 1353629
Flags: needinfo?(amarchesini)
Comment 102•7 years ago
|
||
Which version is that refactoring due for release in (is it 54.0) ?
Flags: needinfo?(keean)
Comment 103•7 years ago
|
||
55. Some of the pblob re-factoring is still landing.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•