1467720 - Can't access dead object in ext-storage.js:168:40

Status: VERIFIED FIXED

(WebExtensions :: Storage, defect, P1)

Description

[Damien Cassou]

Attachment: mozregression.log

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

Steps to reproduce:

- Install passwe addon and app
- open a webpage
- open passwe popup once and choose an entry (it works)
- open passwe popup again


Actual results:

You get "Loading password..." message and nothing happens


Expected results:

You get a list of password entries

Comment 1

[Damien Cassou]

mozregression tells me it is because of https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=885590b1dbd248556b995dfdfab36cf34ead5556&tochange=de1544d92b89fdf13f89d28cb9c8b6b4b2bfbd90.

Comment 2

[Damien Cassou]

The source code for passwe is hosted at https://gitlab.petton.fr/passwe/passwe-addon.

Comment 3

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

Attachment: Bug 1467720 - Fix "dead object" error raised from ext-storage.js on browser.storage.local API calls.

When ExtensionStorageIDB.selectBackend is called from a child process, it calls the main process
and cache the result as a promise, to be reused for the other contexts for the same extension
that are running in the same process.

The cached promise should not be tied to a particular extension context's cloneScope, otherwise
accessing it will raise "can't access dead object" errors after that cloneScope has been
destroyed.

Review commit: https://reviewboard.mozilla.org/r/250210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250210/

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8984386 [details]
Bug 1467720 - Fix "dead object" error raised from ext-storage.js on browser.storage.local API calls.

https://reviewboard.mozilla.org/r/250210/#review256766

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:371
(Diff revision 1)
>        if (context.childManager) {
> +        // Create a promise object that is not tied to the current extension context, because
> +        // we are caching it for the entire life of the extension in the current process (and
> +        // the promise returned by context.childManager.callParentAsyncFunction would become
> +        // a dead object when the context.cloneScope has been destroyed).
> +        promise = new Promise(async (resolve, reject) => {

I think this could be simplified a bit without needing new Promise().  e.g
```
promise = (async () {
  let result = await ...callParent(...);
  if (result.backendEnabled) {
    return { ...result, storagePrincipal: ...};
  } else {
    return {backendEnabled: false};
  }
})();
```

Comment 6

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

Comment on attachment 8984386 [details]
Bug 1467720 - Fix "dead object" error raised from ext-storage.js on browser.storage.local API calls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250210/diff/1-2/

Comment 7

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

Comment on attachment 8984386 [details]
Bug 1467720 - Fix "dead object" error raised from ext-storage.js on browser.storage.local API calls.

https://reviewboard.mozilla.org/r/250210/#review256766

> I think this could be simplified a bit without needing new Promise().  e.g
> ```
> promise = (async () {
>   let result = await ...callParent(...);
>   if (result.backendEnabled) {
>     return { ...result, storagePrincipal: ...};
>   } else {
>     return {backendEnabled: false};
>   }
> })();
> ```

yeah, they do basically the same thing, but using the async function makes it simpler and it also looks better.

change applied!

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8984386 [details]
Bug 1467720 - Fix "dead object" error raised from ext-storage.js on browser.storage.local API calls.

https://reviewboard.mozilla.org/r/250210/#review256852

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6d2c3390c0b
Fix "dead object" error raised from ext-storage.js on browser.storage.local API calls. r=aswan

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c6d2c3390c0b

Comment 11

[marius.santa]

Attachment: loading passwords.jpg

I've tried to reproduce the issue using a fresh profile on the latest nightly build but opened the pop-up and it's stuck loading with the message "Loading passwords...", I think this might be an extension related issue, can you please take a look?
Tested on Windows 10 64Bit in Firefox 62.0a1 (20180618100844).
Attached screenshot.

Comment 12

[Damien Cassou]

> it's stuck loading with the message "Loading passwords..."

the add-on requires an external app (https://gitlab.petton.fr/passwe/passwe-app) as well as a password-store directory.

> can you please take a look?

for me, the bug reported here has been fixed for a few days. I can successfully use my add-on again.

Thanks

Comment 13

[marius.santa]

Marking as verified as per https://bugzilla.mozilla.org/show_bug.cgi?id=1467720#c12.