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

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Debugging a completely different issue I found this bug.
I'll upload a test case asap.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - test

Comment 2

[Andrea Marchesini [:baku]]

Attachment: blob_bug1307747.patch

Comment 3

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

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.

Comment 4

[Pulsebot]

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

Comment 5

[Carsten Book [:Tomcat]]

sorry had to backout for failures liek https://treeherder.mozilla.org/logviewer.html#?job_id=37088206&repo=mozilla-inbound

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7e86ac265a
Backed out changeset c743f069da42 for test failures in test_blob_sliced_from_child_process.html

Comment 8

[Josh Matthews [:jdm]]

empty.html is missing from the patch.

Comment 9

[Andrea Marchesini [:baku]]

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

Comment 10

[Andrea Marchesini [:baku]]

Attachment: blob_bug1307747.patch

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.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: blob_bug1307747.patch

Comment 12

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

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.

Comment 13

[Marcia Knous [:marcia]]

Carrying over the tracking flag from the duplicate bug.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3856bf7772ab
Remote Blob must have the correct size, r=smaug

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3856bf7772ab