1461426 - Messaging blob from background (via IndexedDB) to content corrupts the blob

Status: RESOLVED FIXED

(Core :: DOM: File, defect)

Description

[Anthony Lieuallen]

Attachment: broken-blob.xpi

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180503143129

Steps to reproduce:

https://github.com/greasemonkey/greasemonkey/issues/2943

Greasemonkey tries to A) download a blob, B) persist it to Indexed DB, later C) load it from Indexed DB, later D) pass it to content via messaging.

The experience we have is that some part of this chain corrupts the blob.  Creating a URL from that blob causes a load that never completes.


Actual results:

In real usage, what I see is that when I first try to use a blob in this way (by depending on it from a user script), it works.  At some point later, it stops working.  When I inject the URL generated from the blob into the page, that page never loads (throbber goes forever), and the content (usually CSS) is never loaded/effective.

Attached is the closest repro case I could throw together.  It:

1. Downloads a file via XHR, set to `.responseType = 'blob';`
2. Persists that blob to Indexed DB
3. Loads that blob from Indexed DB
4. Detects web navigations,
5. Does a `tabs.executeScript()`, which
6. Passes a message from content to background side, which
7. Responds with an object holding the blob, so
8. Content side can `URL.createObjectURL(result.blob);` and then
9. Inject a reference to this blob, via URL, into the page


Expected results:

It should have injected that resource (in this case CSS which turns everything red) into the page.  What actually seems to happen is that the background side has loaded the blob just fine (and logs it after FileReader'ing it).  But the content side fails completely.  It can createObjectURL and get a result; it tries to inject a reference to that URL and nothing loads. It tries to FileReader the blob and gets nothing.

Comment 1

[Anthony Lieuallen]

For clarity, to see the exact symptoms we're really having in Greasemonkey:


1. Launch Firefox nightly, new/empty/blank profile
2. Install Grasemonkey
   https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/
3. Install a test script
   https://github.com/greasemonkey/greasemonkey/blob/master/test/scripts/resource-css-geturl.user.js
   (click "Raw")
4. Load any page, see a red dashed border
5. Close and relaunch Firefox
6. Load any page, see no red dashed border, and the page never finishes loading


It's that "close and relaunch" step that causes the bug.  That's what triggers us to actually load the blob out of IndexedDB, which seems (?) to be where the failure really comes from.

Comment 2

[sdaniele3]

Using the .xpi that is provided in the bug report I preformed a mozregression using Windows. As the range decreased to just a few days I started experiencing crashing. As a result I performed two bisections, one for when the crashing started and one for when the crashing stopped. Below are the pushlog links.

Pushlog for when Firefos started crashing.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=df442f1c10332426528aab20ea59d29682183939&tochange=e2357be6f07bd385e4bf7eb3d39885c478c84d24


Pushlog for when Firefox stopped crashing.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=db0bd0d1b8fdd78de2132c4f005e6005ea7a39ce&tochange=06634ddc1a18ca945492a6317f548a8be21dff7b

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Andrea,
the two links in Comment 2 contains a bunch of changes related ti the IPCBlob, do you have any ideas on what could have introduced this issue?

Comment 4

[Anthony Lieuallen]

FYI we've worked around this by storing `ArrayBuffer`s in IndexedDB, and casting those to `Blob`s when reading the values back out:

https://github.com/greasemonkey/greasemonkey/commit/571349a3de1f6c08d75b828e8dce29a9b41a33d5

Comment 5

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Here what happens:

1. The addon downloads, stores and retrieves a blob into/from IDB. Because IDB uses PBackground, the blob is an IPCBlob. All of this runs on the parent process which has IPCBlobInputStreamParent and IPCBlobInputStreamChild actors. The underlying stream is a nsFileStream.

2. The blob is sent to the content process.

3. During the sending, the IPCBlobInputStream is serialized. This means that we create a new IPCBlobInputStreamParent actor and stores the underlying stream (IPCBlobInputStream) into IPCBlobInputStreamStorage. This basically is a loop: IPCBlobInputStreamParent -> IPCBlobInputStream -> IPCBlobInputStreamParent -> nsFileStream.

This is bad because we should not send serialized IPCBlobInputStream from parent to child: the child will not be able to retrieve the underlying stream when needed. There is an assertion to detect this behavior: https://searchfox.org/mozilla-central/source/ipc/glue/InputStreamUtils.cpp#70

In debug build, we crash. In opt build, inputStream will not work because IPCBlobInputStreamStorage, on the content process doesn't contain any inputStream.

The fix here is: when the parent needs to send an IPCBlobInputStream to a content process, it sends the underlying stream instead.

Comment 6

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

We need a test here. All the IPCBlob handling has proved to be error prone, so better to ensure we won't regress this behavior later.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: test

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8981383 [details] [diff] [review]
test

Review of attachment 8981383 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Andrea, follows some review comments on the new test, mostly about cleanups and other small tweaks (and some questions about parts that doesn't seem needed for the test).

(and I'm clearing the review request in the meantime, flag me again on the updated version)

::: toolkit/components/extensions/test/xpcshell/test_ext_ipcBlob.js
@@ +18,5 @@
> +    const blobContent = "Hello World!";
> +
> +    async function openDb() {
> +      if (navigator.storage && navigator.storage.persist) {
> +        await navigator.storage.persist();

Does this test need to turn the storage into persist mode?
if it doesn't we can skip this setup step, otherwise we should probably fail right here is navigator.storage or navigator.storage.persist are not available.

@@ +25,5 @@
> +      return new Promise((resolve, reject) => {
> +        let dbOpen = indexedDB.open(dbName, dbVersion);
> +        dbOpen.onerror = event => {
> +          // Note: can get error here if dbVersion is too low.
> +          console.error('Error opening DB!', event);

If I'm not wrong the actual error should be stored in event.target.error here (same for the error during the db upgrade at line 38).

Also, we can make the error more visible as a failure by using browser.test.fail (e.g. like we do in some other webextension tests https://searchfox.org/mozilla-central/search?q=browser.test.fail&case=false&regexp=false&path=test%2Fxpcshell)

@@ +26,5 @@
> +        let dbOpen = indexedDB.open(dbName, dbVersion);
> +        dbOpen.onerror = event => {
> +          // Note: can get error here if dbVersion is too low.
> +          console.error('Error opening DB!', event);
> +          reject(event);

(as mentioned above, this looks like it should be `reject(event.target.error);`, same for the reject during the db upgrade at line 39)

@@ +43,5 @@
> +      });
> +    }
> +
> +    async function save(blob) {
> +      let db = await openDb();

How about opening the db only once and pass the opened db instance as a parameter to both save and load?

@@ +52,5 @@
> +
> +      return new Promise((resolve, reject) => {
> +        req.onsuccess = event => {
> +          resolve();
> +          load();

It seems that `load` is already being called explicitly right after `await save(...)` at line 86, do we need this additional `load()` call here?

@@ +82,5 @@
> +    }
> +
> +    browser.test.log("Blob creation");
> +    await save(new Blob([blobContent]));
> +    let blob = await load();

If `save` or `load` rejects we could make the entire test to fail immediately (especially because the onMessage listener will never be reached and the "bg-ready" test message will never be sent, and so the test will fail only once it timeouts).

@@ +84,5 @@
> +    browser.test.log("Blob creation");
> +    await save(new Blob([blobContent]));
> +    let blob = await load();
> +
> +    browser.runtime.onMessage.addListener(([msg, what], sender, sendResponse) => {

instead of using the sendResponse parameter, you can just return a promise from the onMessage listener and it will be used to send a response to the sender (and then you can remove both sender and sendResponse from the listener parameters), eg.

```
if (msg === "script-ready") {
  return Promise.resolve({blob});
}
```

@@ +106,5 @@
> +
> +  function contentScriptStart() {
> +    browser.runtime.sendMessage(["script-ready"], response => {
> +      let reader = new FileReader();
> +      reader.addEventListener('load', () => {

I'm wondering if we should also listen for an error event here and exit the test with a failure immediately in that case.

@@ +110,5 @@
> +      reader.addEventListener('load', () => {
> +        browser.runtime.sendMessage(["script-value", reader.result]);
> +      });
> +      reader.readAsText(response.blob);
> +    }); 

Nit, I guess that splinter review tool is detecting a white space here at the end of line 114.

@@ +115,5 @@
> +  }
> +
> +  let extensionData = {
> +    manifest: {
> +      applications: {gecko: {id: "[e-mail]"}},

Do you need to set a particular extension id on this test extension?
it doesn't seem to be used anywhere and it seems that we can just leave the ExtensionTestUtils to generate a random one for us.

@@ +127,5 @@
> +    },
> +    files: {
> +      "content_script_start.js": contentScriptStart,
> +    },
> +    background,

Nit, do you mind to move the background property near the top of the extensionData? (only because it is the central part of the test extension and we usually put it as the first property (before the `manifest` and `files` properties) so that it is immediately visible.

@@ +134,5 @@
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  await extension.startup();
> +
> +  await extension.awaitMessage("bg-ready");
> +  let tc = extension.awaitMessage("test-completed");

There is no need to store the promise returned by awaitMessage before executing the code that is supposed to trigger it,
the abstractions provided by ExtensionTestUtils are collecting and storing the test messages for us, and so you can just
await the message after ExtensionTestUtils.loadContentPage, e.g.

```
let contentPage = await ExtensionTestUtils.loadContentPage(...)

await extension.awaitMessage("test-completed");
```

Also, given that this message is the one that is sent at the end of the test, we could also opt to use here a more explicit:

```
await extension.awaitFinish("test-ipc-blob");
```

and on the other side (e.g. from the background page and/or the content script) we can call:

- `extension.notifyFail`, to signal that the test is already failed from the "test extension" side
- `extension.notifyPass`, to signal that the test is passed (at least from what it concerns the "test extension" side)

(here is a bunch of other test that uses it https://searchfox.org/mozilla-central/search?q=awaitFinish&case=false&regexp=false&path=test%2Fxpcshell, https://searchfox.org/mozilla-central/search?q=notifyFail&case=false&regexp=false&path=test%2Fxpcshell)

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Also, not really mandatory but it would be nice if the test could abstract the promise wrapping of the IDB requests in a way that it can be defined once, and then reused for opening and upgrading the db and for the save and load requests.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: test

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8981405 [details] [diff] [review]
test

Review of attachment 8981405 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/xpcshell/test_ext_ipcBlob.js
@@ +19,5 @@
> +
> +    let db = await new Promise((resolve, reject) => {
> +      let dbOpen = indexedDB.open(dbName, dbVersion);
> +      dbOpen.onerror = () => {
> +        browser.test.fail("Error opening DB!");

Let's include the actual error message here (it may be useful to investigate a test failure if it has been triggered from here).
Same for line 33 and line 51.

@@ +66,5 @@
> +          browser.test.fail("Error loading the blob from the DB!");
> +          browser.test.notifyFail("test-completed");
> +          reject();
> +        }
> +      }).then(async loadDetails => {

This callback doesn't need to be async.

@@ +73,5 @@
> +          blobs.push(details);
> +        });
> +        return blobs[0];
> +      }).catch(err => {
> +        console.error('Failed to load blobs', err);

It seems that this promise's catch callback will be reached if the req.onerror is being executed or if loadDetails is not an array and the forEach at line 72 can't be executed, in the first scenario we already send a browser.test.notifyFail and we don't care about the console.error here, on the second scenario we should also log the original error and make the entire test to fail.

How about move the "browser.test.fail and notifyFail" into the last catch?

Something like:

```
return new Promise((resolve, reject) => {
  req.onsuccess = () => resolve(req.result);
  req.onerror = () => reject(req.error);
}).then(loadDetails => {
 ...
}).catch(err => {
  browser.test.fail("Error loading the blob from the DB: ${err} :: ${err.stack}");
  browser.test.notifyFail("test-completed");
});
```

@@ +95,5 @@
> +        browser.test.notifyPass("test-completed");
> +        return;
> +      }
> +
> +      browser.test.assertTrue(false, "message type is correct");

This assert should be reached only if an unexpected message is being received, we could just use browser.test.fail for it, e.g. 
 
    browser.test.fail(`Unexpected test message received: ${msg}`);

Comment 12

[Andrea Marchesini [:baku]]

Attachment: test

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8981424 [details] [diff] [review]
test

Review of attachment 8981424 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Andrea, looks good to me, we should probably make sure that the test in this version is failing as expected without the fix applied, but I'm sure that you are already verifying that locally.

Follows some additional review comments related to a couple of super small nits, and so I'm setting the r+.

r=me on green try (e.g. let's check that eslint is not going to complain about anything that I may be missed).

::: toolkit/components/extensions/test/xpcshell/test_ext_ipcBlob.js
@@ +43,5 @@
> +      let store = txn.objectStore(dbStore);
> +      let req = store.put(blob, 'key');
> +
> +      return new Promise((resolve, reject) => {
> +        req.onsuccess = event => {

Nit, event is unused here.

@@ +61,5 @@
> +      let req = store.getAll();
> +
> +      return new Promise((resolve, reject) => {
> +        req.onsuccess = () => resolve(req.result);
> +        req.onerror = event => reject(req.error);

Nit, event is unused here

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c3f83a905e
IPCBlobInputStreamParent should be sent as underlying stream to the content, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab1dbec331e
IPCBlobInputStreamParent should be sent as underlying stream to the content - unit test, r=rpl

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d7c3f83a905e
https://hg.mozilla.org/mozilla-central/rev/4ab1dbec331e