Closed Bug 1360807 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: DOM: Workers, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: jseward, Assigned: baku)

Details

(Keywords: crash)

Crash Data

Attachments

(4 files, 3 obsolete files)

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.
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch part 3 - testsSplinter Review
Attachment #8863889 - Flags: review?(bugs)
Attachment #8863891 - Flags: review?(bugs)
Attachment #8863892 - Flags: review?(bugs)
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.
Attachment #8863889 - Flags: review?(bugs) → review-
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.
Attachment #8863891 - Flags: review?(bugs) → review-
Attachment #8863892 - Flags: review?(bugs) → review+
Attachment #8863891 - Attachment is obsolete: true
Attachment #8864104 - Flags: review?(bugs)
Attachment #8864107 - Flags: review?(bugs)
Attachment #8864108 - Flags: review?(bugs)
Attachment #8864108 - Attachment is obsolete: true
Attachment #8864108 - Flags: review?(bugs)
Attachment #8864404 - Flags: review?(bugs)
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.
Attachment #8864104 - Flags: review?(bugs) → review+
Attachment #8864107 - Flags: review?(bugs) → review+
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?
Attachment #8864404 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
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
backed out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=96595044&repo=mozilla-inbound after this and the other bug landed
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
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
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9630a51eca07
FileReaderSync must work with sync inputStream - part 4 - Using BufferedInputStream for Base64EncodeInputStream, r=me
Pushed by amarchesini@mozilla.com:
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
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.
Pushed by amarchesini@mozilla.com:
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
Backout by philringnalda@gmail.com:
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
Backout by kwierso@gmail.com:
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
Pushed by amarchesini@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.