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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Debugging a completely different issue I found this bug.
I'll upload a test case asap.
(Assignee)

Comment 1

2 years ago
Created attachment 8797989 [details] [diff] [review]
part 1 - test
(Assignee)

Comment 2

2 years ago
Created attachment 8797995 [details] [diff] [review]
blob_bug1307747.patch
Attachment #8797989 - Attachment is obsolete: true
Attachment #8797995 - Flags: review?(bugs)

Comment 3

2 years ago
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+

Comment 4

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1307767
sorry had to backout for failures liek https://treeherder.mozilla.org/logviewer.html#?job_id=37088206&repo=mozilla-inbound
Flags: needinfo?(amarchesini)

Comment 6

2 years ago
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.
(Assignee)

Comment 9

2 years ago
(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)
(Assignee)

Comment 10

2 years ago
Created attachment 8798336 [details] [diff] [review]
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.
Attachment #8797995 - Attachment is obsolete: true
Attachment #8798336 - Flags: review?(bugs)
(Assignee)

Comment 11

2 years ago
Created attachment 8798343 [details] [diff] [review]
blob_bug1307747.patch
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.
tracking-firefox52: --- → +

Comment 14

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3856bf7772ab
Remote Blob must have the correct size, r=smaug

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3856bf7772ab
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

2 years ago
Depends on: 1308526
You need to log in before you can comment on or make changes to this bug.