Closed Bug 1307747 Opened 5 years ago Closed 5 years ago

Blob.size is 0 when a big blob is sent from child process to the parent one

Categories

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

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 + fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Debugging a completely different issue I found this bug.
I'll upload a test case asap.
Attached patch part 1 - test (obsolete) — Splinter Review
Attached patch blob_bug1307747.patch (obsolete) — Splinter Review
Attachment #8797989 - Attachment is obsolete: true
Attachment #8797995 - Flags: review?(bugs)
Comment on attachment 8797995 [details] [diff] [review]
blob_bug1307747.patch


>+++ b/dom/base/test/browser_bug1307747.js
>@@ -0,0 +1,32 @@
>+/* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; js-indent-level: 2 -*- */
>+
>+var {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
>+
>+const BASE_URI = "http://mochi.test:8888/browser/dom/base/test/empty.html";
>+
>+add_task(function* test_initialize() {
>+  let tab = gBrowser.addTab(BASE_URI);
>+  let browser = gBrowser.getBrowserForTab(tab);
>+
>+  let blob = yield ContentTask.spawn(browser, null, function() {
>+    let blob = new Blob([new Array(1024*1024).join('123456789ABCDEF')]);
>+    return blob;
>+  });
I have no idea what this does, but somehow the outer blob variable gets the inner blob


>+
>+  ok(blob, "We have a blob!");
>+  is(blob.size, new Array(1024*1024).join('123456789ABCDEF').length, "The size matches");
Hopefully all the large arrays don't cause OOMs while running tests.


>-  uint64_t available;
>-  MOZ_ALWAYS_SUCCEEDS(inputStream->Available(&available));
>-
>   RefPtr<BlobImpl> blobImpl;
>   if (!aMetadata.mHasRecursed && aMetadata.IsFile()) {
>-    if (available) {
>+    if (aMetadata.mLength) {
>       blobImpl =
>         new BlobImplStream(inputStream,
>                            aMetadata.mName,
>                            aMetadata.mContentType,
>                            aMetadata.mLastModifiedDate,
>-                           available);
>+                           aMetadata.mLength);
>     } else {
>       blobImpl =
>         new EmptyBlobImpl(aMetadata.mName,
>                           aMetadata.mContentType,
>                           aMetadata.mLastModifiedDate);
>     }
>-  } else if (available) {
>+  } else if (aMetadata.mLength) {
>     blobImpl =
>-      new BlobImplStream(inputStream, aMetadata.mContentType, available);
>+      new BlobImplStream(inputStream, aMetadata.mContentType,
>+                         aMetadata.mLength);


Ok, based on IRC discussion this should be right. 
But I'm quite worried that other use of Available may have similar issues. We need to audit them all.
This is super error prone setup. Occasionally Available works , occasionally it doesn't.

Please file a bug sorting out this all, or at least to audit all Available use.
Attachment #8797995 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c743f069da42
Remote Blob should use the real size of the blob and not what available from the inputStream, r=smaug
Blocks: 1307767
sorry had to backout for failures liek https://treeherder.mozilla.org/logviewer.html#?job_id=37088206&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7e86ac265a
Backed out changeset c743f069da42 for test failures in test_blob_sliced_from_child_process.html
Duplicate of this bug: 1307462
empty.html is missing from the patch.
(In reply to Josh Matthews [:jdm] from comment #8)
> empty.html is missing from the patch.

No, it's not. but splinter doesn't show it because it's... 'empty'.
Flags: needinfo?(amarchesini)
Attached patch blob_bug1307747.patch (obsolete) — Splinter Review
This is a bit more complex than before. The issue is this:

before sending IPCStream for child blobs, we were serializing all the content into a buffer and this buffer was sent to the parent. This means that if we had a MultipartBlobImpl, we were still serializing all in 1 single buffer.

Now, we create multiple BlobData for each sub-Blob (see BlobDataFromBlobImpl in dom/ipc/Blob.cpp). But at the same time, we take the size of the blob (in total) considering also the size of each sub-blob. When we recreate the Blob in the parent process, we take that size and we use it for each sub-blob. (the bug).

In my patch I keep the size of the single IPCStream together with the stream.

I want to file a follow up to cleanup all this blob creation. There are too many types and I guess we can make everything easier to follow.
Attachment #8797995 - Attachment is obsolete: true
Attachment #8798336 - Flags: review?(bugs)
Attachment #8798336 - Attachment is obsolete: true
Attachment #8798336 - Flags: review?(bugs)
Attachment #8798343 - Flags: review?(bugs)
Comment on attachment 8798343 [details] [diff] [review]
blob_bug1307747.patch

CreateBlobImpl should be fixed so that it doesn't take unrelated metadata as a param. But that can be done in a followup.
Attachment #8798343 - Flags: review?(bugs) → review+
Carrying over the tracking flag from the duplicate bug.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3856bf7772ab
Remote Blob must have the correct size, r=smaug
https://hg.mozilla.org/mozilla-central/rev/3856bf7772ab
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1308526
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.