1360807 - Crash in `anonymous namespace''::EncodeInputStream<T>

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Julian Seward [:jseward]]

This bug was filed from the Socorro interface and is 
report bp-fcae6517-50fa-4ae0-bdc6-90e810170429.
=============================================================

This is #12 topcrash in the Windows nightly of 20170427030231.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsIConverterInputStream needs a syncInputStream

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - Base64EncodeInputStream needs a sync inputStream

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8863889 [details] [diff] [review]
part 1 - nsIConverterInputStream needs a syncInputStream

We should try to avoid the extra mem copy whenever possible.
And this needs some good comments in the code why we need to do this copying.

I'm worried that other stream usage has similar issues. Please audit other code.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8863891 [details] [diff] [review]
part 2 - Base64EncodeInputStream needs a sync inputStream

>-  uint32_t numRead;
>-  aRv = Read(stream, bufferData.get(), blobSize, &numRead);
>-  if (NS_WARN_IF(aRv.Failed())) {
>+  char* buffer = bufferData.get();
>+  uint64_t pos = 0;
>+
>+  while (pos < blobSize) {
>+    uint32_t numRead;
>+    aRv = Read(stream, buffer + pos, blobSize - pos, &numRead);
>+    if (NS_WARN_IF(aRv.Failed())) {
>+      return;
>+    }
>+
>+    if (numRead == 0) {
>+      break;
>+    }
>+
>+    pos += numRead;
>+  }
>+
This looks wrong. Read should deal with looping.
If Read has so far been reliable even with multiplex streams, it should be reliable now too.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 0 - SyncRead should read as much as required

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsIConverterInputStream needs a syncInputStream

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - Base64EncodeInputStream needs a sync inputStream

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 2 - Base64EncodeInputStream needs a sync inputStream

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8864104 [details] [diff] [review]
part 0 - SyncRead should read as much as required

>+  // All good.
>+  if (NS_SUCCEEDED(rv)) {
>+    // Not enough data, let's read recursively.
Loop would be nicer, hopefully we don't end up doing too deep recursive calls.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8864404 [details] [diff] [review]
part 2 - Base64EncodeInputStream needs a sync inputStream

>-  uint64_t size = aBlob.GetSize(aRv);
>+  nsCOMPtr<nsIInputStream> stringStream;
Why the name stringStream here? Would syncStream work?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8275594fe235
FileReaderSync must work with sync inputStream - part 1 - SyncRead should read as much as required, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b389f6ad971
FileReaderSync must work with sync inputStream - part 2 - nsIConverterInputStream needs a syncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d43275b33f00
FileReaderSync must work with sync inputStream - part 2 - tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/40fcf8d32f3a
FileReaderSync must work with sync inputStream - part 3 - Base64EncodeInputStream needs a sync inputStream, r=smaug

Comment 13

[Carsten Book [:Tomcat]]

backed out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=96595044&repo=mozilla-inbound after this and the other bug landed

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f838f22efa
Backed out changeset 40fcf8d32f3a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/e246b7e6278a
Backed out changeset d43275b33f00 
https://hg.mozilla.org/integration/mozilla-inbound/rev/146f3b415f6c
Backed out changeset 6b389f6ad971 
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d2a49c6b5f
Backed out changeset 8275594fe235

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab091bdeda1
FileReaderSync must work with sync inputStream - part 1 - SyncRead should read as much as required, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c800cb830b36
FileReaderSync must work with sync inputStream - part 2 - nsIConverterInputStream needs a syncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59616a3997a
FileReaderSync must work with sync inputStream - part 2 - tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e9af8078dd
FileReaderSync must work with sync inputStream - part 3 - Base64EncodeInputStream needs a sync inputStream, r=smaug

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9630a51eca07
FileReaderSync must work with sync inputStream - part 4 - Using BufferedInputStream for Base64EncodeInputStream, r=me

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a05e05fb644
FileReaderSync must work with sync inputStream - part 5 - Cleaning up the use of BufferedInputStream for Base64EncodeInputStream, r=me

Comment 18

[Phil Ringnalda (:philor)]

Backed out all six in https://hg.mozilla.org/integration/mozilla-inbound/rev/797a743271c6 for continuing hangs in test_fileReader.html and test_ipcBlob_fileReaderSync.html, https://treeherder.mozilla.org/logviewer.html#?job_id=96749173&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=96749226&repo=mozilla-inbound

e10s asan seems the most prone to hitting it, but linux64 opt and pgo were also willing to, with a little less frequency.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25d7a76008e
FileReaderSync must work with sync inputStream - part 1 - SyncRead should read as much as required, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e84dfab90bf
FileReaderSync must work with sync inputStream - part 2 - nsIConverterInputStream needs a syncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/50d6d6733209
FileReaderSync must work with sync inputStream - part 3 - tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7790c232791
FileReaderSync must work with sync inputStream - part 4 - Base64EncodeInputStream needs a sync inputStream, r=smaug

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0f7ac46e1f
Backed out 4 changesets for a 30% failure rate in test_ipcBlob_fileReaderSync.html on ASan e10s

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/4e16ec1a2144
Backed out 4 changesets for a 30% failure rate in test_ipcBlob_fileReaderSync.html on ASan e10s a=merge

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d38bbbb10b
FileReaderSync must work with sync inputStream - part 1 - SyncRead should read as much as required, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80ae4ab872f
FileReaderSync must work with sync inputStream - part 2 - nsIConverterInputStream needs a syncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8def8bb39558
FileReaderSync must work with sync inputStream - part 3 - tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b8a2a92704
FileReaderSync must work with sync inputStream - part 4 - Base64EncodeInputStream needs a sync inputStream, r=smaug

Comment 23

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=622ca2d507b9f46b0f9f5f4ece7bbc314418ff29&selectedJob=97577896

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/21d38bbbb10b
https://hg.mozilla.org/mozilla-central/rev/d80ae4ab872f
https://hg.mozilla.org/mozilla-central/rev/8def8bb39558
https://hg.mozilla.org/mozilla-central/rev/86b8a2a92704