Crash in memcpy | NS_CopySegmentToBuffer rising in Firefox 49

RESOLVED DUPLICATE of bug 1332587

Status

()

Core
DOM
--
critical
RESOLVED DUPLICATE of bug 1332587
11 months ago
9 months ago

People

(Reporter: philipp, Assigned: baku)

Tracking

({crash})

49 Branch
mozilla53
x86
Windows
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51+ fixed, firefox52 fixed, firefox53 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
+++ This bug was initially created as a clone of Bug #1294533 +++

a patch that attempted to fix crashes with the signature [@ memcpy | NS_CopySegmentToBuffer] has gone into 51.0b13, but reports with this signature are still continuing for users on that version: http://bit.ly/2if11Bw

Comment 1

11 months ago
I thought perhaps the FileReader could be CC'd while the pipe is being read, but it seems that Shutdown() should clear the mAsyncStream here:

https://dxr.mozilla.org/mozilla-central/source/dom/file/FileReader.cpp#738

Which would then stop OnInputStreamAvailable here:

https://dxr.mozilla.org/mozilla-central/source/dom/file/FileReader.cpp#599

Maybe we should just move or duplicate this line right before the mAsyncStream->Read():

https://dxr.mozilla.org/mozilla-central/source/dom/file/FileReader.cpp#332

And maybe try to add some diagnostic asserts to figure out why its ending up nullptr there.

Andrea, what do you think?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

10 months ago
Created attachment 8826127 [details] [diff] [review]
fr.patch

I really hope that Close() doesn't spin the event loop. But in case, this patch, makes Shutdown() able to deal with it.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8826127 - Flags: review?(bkelly)

Comment 3

10 months ago
Comment on attachment 8826127 [details] [diff] [review]
fr.patch

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

I'm sorry, I don't understand how this is supposed to fix the crash.  Also, we can see in the stacks that the event loop is not being spun:

https://crash-stats.mozilla.com/report/index/40d088ca-334e-4067-bf61-d45a62170112

Can we just add some extra nullptr checks before the Read() call?  We can MOZ_DIAGNOSTIC_ASSERT() to try to figure it out in nightly/aurora, but at least we won't crash in release channel.
Attachment #8826127 - Flags: review?(bkelly)
(Assignee)

Comment 4

10 months ago
If shutdown happens before the read, changing the readyState produces a quick return in the read method.
Flags: needinfo?(bkelly)

Comment 5

10 months ago
Comment on attachment 8826127 [details] [diff] [review]
fr.patch

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

I'm sorry, I don't understand how this is supposed to fix the crash.  Also, we can see in the stacks that the event loop is not being spun:

https://crash-stats.mozilla.com/report/index/40d088ca-334e-4067-bf61-d45a62170112

Can we just add some extra nullptr checks before the Read() call?  We can MOZ_DIAGNOSTIC_ASSERT() to try to figure it out in nightly/aurora, but at least we won't crash in release channel.
Attachment #8826127 - Flags: review+

Comment 6

10 months ago
Ok, lets try it.
Flags: needinfo?(bkelly)
(Assignee)

Comment 7

10 months ago
Yes, I add MOZ_DIAGNOSTIC_ASSERT as well.

Comment 8

10 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31883e2369a
Add some security checks about the use of the string buffer in FileReader, r=bkelly

Comment 9

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d31883e2369a
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 10

10 months ago
should we try to uplift the prospective fix again?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 11

10 months ago
Comment on attachment 8826127 [details] [diff] [review]
fr.patch

Approval Request Comment
[Feature/Bug causing the regression]: FileReader API
[User impact if declined]: a crash can occur.
[Is this code covered by automated tests?]: no, because it's racy to reproduce.
[Has the fix been verified in Nightly?]: hard to verify because racy.
[Needs manual test from QE? If yes, steps to reproduce]: read above.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no.
[Why is the change risky/not risky?]: The Shutdown() method sets the readyState to DONE in order to avoid nested Read().
[String changes made/needed]: none

We want to land this patch for testing: we want to see if the read() happens after the Shutdown(). In case this is the reason of the crash, this patch fixes this bug.
Flags: needinfo?(amarchesini)
Attachment #8826127 - Flags: approval-mozilla-beta?
Attachment #8826127 - Flags: approval-mozilla-aurora?
Comment on attachment 8826127 [details] [diff] [review]
fr.patch

speculative crash fix with added diagnostics, aurora52+
Attachment #8826127 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It seems rather late to push a speculative fix to beta right before RC week, but I'll let Liz or Gerry make that call.

Comment 14

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/43b203270b27
status-firefox52: affected → fixed
I'll check back over the weekend to see if the crash signature keeps happening on nightly. We will still have some late uplifts to beta over the weekend I'm sure, to get this into 51 before the merge.
tracking-firefox51: --- → +
baku if you think of it, and have time, can you follow up over the weekend, to check for related crashes in socorro and comment here?
Flags: needinfo?(amarchesini)
Flags: needinfo?(lhenry)
Comment on attachment 8826127 [details] [diff] [review]
fr.patch

Let's go ahead and land this for the 51 RC build, now that I look over it again, the crash signature wasn't showing up on Nightly - only on beta and release. We may be able to test with the STR from bug 1314046.
Flags: needinfo?(lhenry)
Attachment #8826127 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
(Reporter)

Comment 18

10 months ago
oh, that reminded me that the crash had a way to be reproduced - after the patch here in this bug firefox is still crashing when trying to attempt to attach a big file in yahoo mail, but with a different signature now: https://crash-stats.mozilla.com/report/index/91539763-51b4-470d-9055-3c54a2170114
Crash Signature: [@ memcpy | NS_CopySegmentToBuffer] → [@ memcpy | NS_CopySegmentToBuffer] [@ mozilla::dom::FileReader::DoReadData]
https://hg.mozilla.org/releases/mozilla-beta/rev/ce1b91e82c55
status-firefox51: affected → fixed
(Assignee)

Updated

10 months ago
Blocks: 1331340
(Assignee)

Comment 20

10 months ago
I think bug 1331340 fixes this issue.
(Assignee)

Updated

10 months ago
Flags: needinfo?(amarchesini)
This crash is still reproducible using STR mentioned in comment 18 on 51.0-build1 (20170116133120), latest Aurora 52.0a2 (2017-01-17) and latest Nightly with the same signature: https://crash-stats.mozilla.com/report/index/33b4013c-2b7d-4bce-b9be-1d7752170117
 - Tested on Windows 10 and 7 x86 (not reproducible on x64)

Also, I can see this issue under Ubuntu 14.04 x86, but seems to have a different signature:
https://crash-stats.mozilla.com/report/index/d3056d93-4b39-4231-be8d-3a5582170117

What do you think about this, Andrea?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 22

10 months ago
I submitted a new patch in bug 1331340. Can you test it in nightly again tomorrow? Thanks.
Flags: needinfo?(amarchesini) → needinfo?(ciprian.georgiu)
Tested again with latest Nightly 53.0a1 (2017-01-19) (32-bit) and I can still reproduce it.
https://crash-stats.mozilla.com/report/index/d962c163-6bf8-4c34-aff6-79c5b2170119
Flags: needinfo?(ciprian.georgiu)
(Assignee)

Comment 24

10 months ago
I need to know how you can reproduce it. Do you have a testcase? or a page? Thanks.
Flags: needinfo?(ciprian.georgiu)
(In reply to Andrea Marchesini [:baku] from comment #24)
> I need to know how you can reproduce it. Do you have a testcase? or a page?

To reproduce this, I simply attached a big file ~5 Gb (downloaded from here: https://goo.gl/jKs537) in yahoo email. You can see better here in a gif: https://goo.gl/JupOap. 

Please let me know if I can help you with more info.
Flags: needinfo?(ciprian.georgiu)
(Assignee)

Comment 26

10 months ago
Wow, thanks a lot for this STR!
I'll file a new bug and I'll work on it today.
(Assignee)

Updated

10 months ago
Blocks: 1332587
Resolution: FIXED → DUPLICATE
Duplicate of bug: 1332587
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.