1330273 - Crash in memcpy | NS_CopySegmentToBuffer rising in Firefox 49

Status: RESOLVED DUPLICATE of bug 1332587

(Core :: DOM: Core & HTML, defect)

Description

[[:philipp]]

+++ 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

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 2

[Andrea Marchesini [:baku]]

Attachment: 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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Andrea Marchesini [:baku]]

If shutdown happens before the read, changing the readyState produces a quick return in the read method.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Ok, lets try it.

Comment 7

[Andrea Marchesini [:baku]]

Yes, I add MOZ_DIAGNOSTIC_ASSERT as well.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d31883e2369a

Comment 10

[[:philipp]]

should we try to uplift the prospective fix again?

Comment 11

[Andrea Marchesini [:baku]]

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.

Comment 12

[Julien Cristau [:jcristau]]

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

speculative crash fix with added diagnostics, aurora52+

Comment 13

[Julien Cristau [:jcristau]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43b203270b27

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 18

[[:philipp]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ce1b91e82c55

Comment 20

[Andrea Marchesini [:baku]]

I think bug 1331340 fixes this issue.

Comment 21

[Ciprian Georgiu, Desktop QA]

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?

Comment 22

[Andrea Marchesini [:baku]]

I submitted a new patch in bug 1331340. Can you test it in nightly again tomorrow? Thanks.

Comment 23

[Ciprian Georgiu, Desktop QA]

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

Comment 24

[Andrea Marchesini [:baku]]

I need to know how you can reproduce it. Do you have a testcase? or a page? Thanks.

Comment 25

[Ciprian Georgiu, Desktop QA]

(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.

Comment 26

[Andrea Marchesini [:baku]]

Wow, thanks a lot for this STR!
I'll file a new bug and I'll work on it today.