Closed Bug 1477015 Opened 6 years ago Closed 6 years ago

Select the storage.local backend during the extension startup

Categories

(WebExtensions :: Storage, defect, P1)

Product:
WebExtensions
Component:
Storage
Type:
defect

Tracking

(firefox61 unaffected, firefox62 verified, firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla63
Iteration:
63.3 - Aug 6
Tracking Flags:
Tracking Status
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
aswan
: review+
Details
@storage-local-set-from-destroying-popup.xpi
5.91 KB, application/x-xpinstall
Details
Bug1477015.png
457.04 KB, image/png
Details
fx62beta-Bug_1477015___Select_the_storage_local_backend_during_the_extension_startup_when_ExtensionStorageIDB_is_disabled_.patch
8.48 KB, patch
aswan
: review+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review
Postfix video Beta
233.79 KB, image/gif
Details 
The goal of this bugzilla issue is to evaluate how much simpler and less error-prone would be the implementation if we would select the storage.local backend during the extension startup (and if we still think that it could introduce an "user perceivable performance issue" during the browser startup, try to actually measure it and compare it on the two approaches).

----

As discussed yesterday with Shane over IRC and Vidyo, I'd like to evaluate the possibility to go back to select the storage.local backend for an extension during the extension startup (as it was in the previous versions of the patches from Bug 1406181) instead of selecting it when the storage.local API has been called for the first time by the extension (as it is currently).

One of the reasons is that the current "lazy" approach has still issues (independently from which of the backend is actually selected) with extensions that call the storage.local.set API method from a "context that is being destroyed" (e.g. an extension popup, which can be closed right after the method has been called even without an explicit `window.close()` called from the extension itself, given that the extension popups are closed automatically when they lose focus).

The other one is that many of the issues we have been fixing on the code that selects the storage.local backend were related to the "lazy backend selection" when the extension calls the storage.local API for the first time, which makes the implementation actually much more complex and error-prone than how much it would be if the backend would be selected upfront during the extension startup and propagate to the child processes (so that all the extension contexts immediately know which backend has been selected for the extension, without the need to exchange messages with the main process during the first storage.local call).

In the end, most of the extensions are supposed to be be migrated to the new storage.local backend only once, and after being switched to the IndexedDB backend, there is not going to be any actual additional work during all the next extension startups.

Also, if we are still worried that the backend selection (and data migration) would introduce any "user perceivable startup performance issues", we should try to actually measure it, to be sure that the same "user perceivable performance" issue is not actually there even with the current lazy approach.

As an example:
an extension that use both the storage.local and webRequest blocking APIs, e.g. AdBlock Plus, is likely that it would be reading the storage.local data before being able to unblock any HTTP requests that have been blocked during the browser startup, and so there could be a similar "user perceived performance issue" even with the lazy "storage.local backend selection" approach that we are currently using.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
I had to also take a look at how much the same kind of change would differ to be possible to apply it on beta (Firefox 62).

The only real changes needed have been the ones related to usage of the extension.setSharedData/extension.getSharedData methods which were not yet available in Firefox 62 (and so the version of the patch for beta uses the initialProcessData to propagate the selected storage.local backend and the storagePrincipal to the child processes).

I'm attaching the resulting patch as a reference of the differences between the patches that could be applied on Firefox 63 and 62.
Iteration: --- → 63.3 - Aug 6
Blocks: 1474562
Attachment #8993425 - Flags: review?(mixedpuppy)
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

https://reviewboard.mozilla.org/r/258176/#review266324
Attachment #8993425 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

Yesterday I briefly discussed the approach proposed in the previous version of the patch with Andrew (as I agreed with Shane last week) to double-check the impact that "waiting the data migration to be completed before when the extension is started" could have for an extension that also have some "persisted event listeners" to be able to intercept webRequests that are happening while the extension is starting (e.g. AdBlock Plus is definitely one of those).

It turned out that the proposed approach needs some further changes, because we handle this when we are about to create the background page (https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/toolkit/components/extensions/parent/ext-backgroundPage.js#68).

The new version of the patch:

- pre-select the storage.local backend only when:
  - the IDB backend is completely disabled ("extensions.webextensions.ExtensionStorageIDB.enabled" preference is false)
  - or the extension has been already migrated to the IDB backend 
    ("extensions.webextensions.ExtensionStorageIDB.migrated.EXTENSION_ID" preference is true)

- if a data migration is needed, schedule it to run "on idle" during the extension startup (but without waiting it to be completed)  

- fallback to run the storage.local API call on the main server if the storage.local backend is still unknown,
  and the API method may be changing the stored data (and so not for storage.local.get)

I'm clearing the currently set r+ and asking a new review round (to both Shane, so that he can check if there is anything in the new version that may change his previous r+ or requires new review comments, and Andrew to double check the details that we have discussed and that the new proposed approach is going to play well with how we handle the "delayed background page startup").
Attachment #8993425 - Flags: review?(mixedpuppy)
Attachment #8993425 - Flags: review?(aswan)
Attachment #8993425 - Flags: review+
Attachment #8993686 - Attachment is obsolete: true
Priority: -- → P1
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

https://reviewboard.mozilla.org/r/258176/#review266978

::: toolkit/components/extensions/Extension.jsm:1784
(Diff revision 6)
> +          this.setSharedData("storageIDBBackend", false);
> +        } else if (ExtensionStorageIDB.isMigratedExtension(this)) {
> +          this.setSharedData("storageIDBBackend", true);
> +          this.setSharedData("storageIDBPrincipal", ExtensionStorageIDB.getStoragePrincipal(this));
> +        } else {
> +          // Start the data migration once idle.

Might be good to note that runManifest will run prior to idle, and must run prior to selectBackend.  A small part of me wants to suggest s/selectBackend/migrateBackend/
Attachment #8993425 - Flags: review?(mixedpuppy) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> Might be good to note that runManifest will run prior to idle, and must run
> prior to selectBackend.  

Good point, let's make sure of that by scheduling that arrow function to run on idle only after the Extension instance has fired the "ready" message once (I've updated the patch accordingly in the last push to mozreview). 

> A small part of me wants to suggest s/selectBackend/migrateBackend/

Eh, I was also tempted to do that, but that method is doing both (it migrate the data only if needed and then it select the backend) and I'm not sure that renaming the method would help to make that more clear.
Attachment #8993425 - Flags: review?(aswan)
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

https://reviewboard.mozilla.org/r/258176/#review267356

I may be overlooking something but I think this would be overall simpler and easier to follow if the logic for deciding when to migrate was implemented in the parent process.  That would mean all the child-side logic for calling getStorageBackend() could go away.  This child side would choose the child-side implementation initially in the same way it currently does, but then the result of any request on the json backend could be "change to the idb backend and re-execute this request".  That should still address the calling-set-while-a-popup-is-closing scenario and I suspect the actual implementation would be considerably simpler.

::: toolkit/components/extensions/child/ext-storage.js:141
(Diff revision 7)
>        }
>        return sanitized;
>      }
>  
> -    // Detect the actual storage.local enabled backend for the extension (as soon as the
> -    // storage.local API has been accessed for the first time).
> +    function fireOnChanged(changes) {
> +      Services.cpmm.sendAsyncMessage(`Extension:StorageLocalOnChanged:${extension.uuid}`, changes);

Why change away from callParentFunction?

::: toolkit/components/extensions/child/ext-storage.js:167
(Diff revision 7)
>      };
>  
> +    // Synchronously select the backend if it is already known.
> +    let selectedBackend;
> +
> +    switch (extension.getSharedData("storageIDBBackend")) {

nit: why switch instead of just if?

::: toolkit/components/extensions/child/ext-storage.js:187
(Diff revision 7)
>      // Generate the backend-agnostic local API wrapped methods.
>      const local = {};
>      for (let method of ["get", "set", "remove", "clear"]) {
>        local[method] = async function(...args) {
>          try {
> +          let backend = selectedBackend;

why is this extra variable needed?
Attachment #8993425 - Flags: review?(aswan)
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

https://reviewboard.mozilla.org/r/258176/#review267356

> Why change away from callParentFunction?

The reason is that callParentFunction is tied to the extension context that has originated the API call, and if you take a look at where `fireOnChanged` is used you will notice that it is going to wait for two asynchronous IndexedDB operations to be completed, eg.:

```
        const db = await getDB();
        const changes = await db.remove(keys);

        if (changes) {
          fireOnChanged(changes);
        }
```

When the API call is being called from a context that is being destroyed, the context may be destroyed at any time between the two `await` resolve, and so if we try to send the changes to the main process using `callParentFunction` there are very good chances that the context is already destroyed (the one that originated tha call in the child, or the "proxy context" that represent it in the parent process) and the onChanged event ignored (while the changes has been applied to the stored data just fine as expected).

Even the test case included in this patch is able to trigger it intermittently (it is a race and so it is pretty "timing dependent").
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

(In reply to Andrew Swan [:aswan] from comment #13)
> Comment on attachment 8993425 [details]
> Bug 1477015 - Select storage.local backend on startup when the extension is
> not migrating its data.
> 
> https://reviewboard.mozilla.org/r/258176/#review267356
> 
> I may be overlooking something but I think this would be overall simpler and
> easier to follow if the logic for deciding when to migrate was implemented
> in the parent process.  That would mean all the child-side logic for calling
> getStorageBackend() could go away.  This child side would choose the
> child-side implementation initially in the same way it currently does, but
> then the result of any request on the json backend could be "change to the
> idb backend and re-execute this request".  That should still address the
> calling-set-while-a-popup-is-closing scenario and I suspect the actual
> implementation would be considerably simpler.

The "logic for deciding when to migrate" is already happening only in the parent process, the child processes just ask to the parent process "which one is the selected backend" (e.g. to be sure that the backend selection is only done once and all the extension contexts are going to use the same backend, even if the about:config preferences has been changed in the meantime).

Having said that, the IndexedDB backend can run mostly in the child process (the only part that currently require the main process is "firing the onChanged event"), whereas the JSONFile backend has to forward all the API calls to the main process where they are going to be processed.

If I'm reading correctly your comment, you are suggesting to forward all the storage.local API calls to the main process (e.g. by using the JSONFile backend) if the backend is not already known during the extension startup.
Then in the parent process, when we receive this forwarded calls, we check the outcome of the data migration and we decide where to write the data.

am I right?

If that's the approach suggested, then yes it could work (after all it is pretty similar to what this patch is currently doing, besides for the storage.local.get), but:

- I'm not really sure that it would make the "child side `getStorageBackend()` logic to go completely away", if we don't want to keep forwarding all the API calls to the main process until the extension is restarted again (and the backend is already known during the extension startup).

- it seems to me that the suggested approach is going to resort to the main process much more during the migration (which we may even decide that it is not a big deal, because in the end it is what the current JSONFile backend does, but it still seems to be worth a mention, e.g. in case I've been misreading your suggestion)

"Resorting to the main process for the storage.local API calls" is one of the things that make the current backend more "expensive" for the main process than the IndexedDB one, and so I have been trying to minimize the amount of requests we forward to the main process while we are still migrating and we don't know yet the selected backend. 

e.g. in the attached patch:

- the implementation does not forward to the main process any storage.local.get API calls, because it doesn't really matter if that API calls fails because the extension context that originated the call has been destroyed (in the end that context was the only one interested in the outcome of the storage.local.get call, and once it is gone, the result of the API call is not really needed)

- but it does forward the other API calls, because they are supposed to apply some changes to the stored data and then fire the onChanged event, and so it should succeed even if the context that has originated the call is destroyed (because other extension contexts may be interested in and they may be expecting those outcomes)
Attachment #8993425 - Flags: review?(aswan)
The following screencast shows an experiment that I've been running locally to double-check the impact that the storage.local API and the data migration may have on the "user perceived startup performance".

In this experiment, Firefox is starting up with AdBlock Plus installed and configured with a considerable amount of filters (which are around ~49Mb of data stored in the json file), initially Firefox is started configured to use the JSONFile backend, then it is started again while migrating to the IndexedDB backend and one more time once the extension is already migrated to the IndexedDB backend:

https://youtu.be/kblmHBmIcnY

AdBlock Plus processes the filters from the extension code right after reading them from storage.local, and so there is still an amount of latency and the extension process becomes busy for a while while the filters are being processed, but it also seems that the IndexedDB backend has still a visible positive impact in the "user perceived startup performance" (e.g. when Firefox is started with a webpage, the page starts to load and is rendered much sooner when the extension is using the IndexedDB backend, even when the extension is being migrating its data between the JSONFile and the IndexedDB backend).
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

https://reviewboard.mozilla.org/r/258176/#review267572

::: toolkit/components/extensions/child/ext-storage.js:140
(Diff revision 9)
>          sanitized[key] = ExtensionStorage.sanitize(value, context);
>        }
>        return sanitized;
>      }
>  
> -    // Detect the actual storage.local enabled backend for the extension (as soon as the
> +    function fireOnChanged(changes) {

Please add a comment explaining that we're using the underlying message manager since the parent context may be gone by the time we call this so we can't use callParentFunction()

::: toolkit/components/extensions/child/ext-storage.js:210
(Diff revision 9)
> +              const result = await context.childManager.callParentAsyncFunction(
> +                "storage.local.callMethodInParentProcess", [method, args]);
> +              return result;
> +            }
> +
> +            selectedBackend = await promiseStorageLocalBackend;

Seems like you should keep either this or line 190 but not both...

::: toolkit/components/extensions/parent/ext-storage.js:45
(Diff revision 9)
> +    super(extension);
> +
> +    if (ExtensionStorageIDB.isBackendEnabled) {
> +      const messageName = `Extension:StorageLocalOnChanged:${extension.uuid}`;
> +      Services.ppmm.addMessageListener(messageName, this);
> +      clearStorageChangedListeners.set(this, () => {

why does this need to be in a Map rather than just a property on the api object?
Attachment #8993425 - Flags: review?(aswan) → review+
Comment on attachment 8993425 [details]
Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data.

https://reviewboard.mozilla.org/r/258176/#review267572

> Please add a comment explaining that we're using the underlying message manager since the parent context may be gone by the time we call this so we can't use callParentFunction()

Sure thing, I added an inline comment in the updated version.

> Seems like you should keep either this or line 190 but not both...

ouch, that's true, and I should have removed it when I've removed the local backend variable in the previous update of this patch, thanks for pointing it out! I opted to remove the `.then(...)` part at line 190.

> why does this need to be in a Map rather than just a property on the api object?

It can definitely be a property of the API object, I changed it in the last updated version to set the "clear" function as a object property and removed the WeakMap.

While looking at it I also noticed that checking the `ExtensionStorageIDB.isBackendEnabled` property here may not be a good idea (because it may be changing its value if the preference is changing while the extension is starting, eg. by the user, and at the same time the storage's ExtensionAPI instance is being created in the main process), and so I removed that check.
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/d7aac87e2db5
Select storage.local backend on startup when the extension is not migrating its data. r=aswan,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/d7aac87e2db5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
I'm attaching a small test extension to more easily verify this issue, by using the following STR (to be executed on the JSONFile backend and then again on the IndexedDB backend):

- Install the test extension temporarily

- open about:config and check that "extensions.webextensions.ExtensionStorageIDB.enabled" is set to false, to verify the fix on the JSONFile backend (then repeat the same STR with this pref set to true, to verify the same behavior on the IndexedDB backend)

- open the Browser Console (and clear all the messages)

- open the test extension browserAction popup and press the "storage.local.set and close" button

- in the browser console, look for a logged console message: STORAGE.ONCHANGED local Object { "test-key": {…} }

- open the test extension browserAction popup and press the "storage.local.get. and wait" button

- in the browser console, look for the following two logged console message: 
  - CALLING storage.local.get and wait for its result
  - RESOLVED storage.local.get Object { "test-key": "value set from destroying context" }
Flags: qe-verify+
Attached image Bug1477015.png — 
This issue is verified as fixed on Firefox 63.0a1 (20180802100128) under Win 7 64-bit and Mac OS X 10.13.3.

Firefox 62.0b13 (20180730180407) under Win 7 64-bit and Mac OS X 10.13.3 is still affected.

Firefox 61.0.1 (20180704003137) under Win 7 64-bit and Mac OS X 10.13.3  is not affected.

Please see the attached screenshot.
Status: RESOLVED → VERIFIED
status-firefox61: --- → unaffected
status-firefox62: --- → affected
status-firefox63: fixed → verified
Flags: qe-verify+
uBlock Origin had issue with resetting its own settings - 
clearing storage.local and then restarting did not restore default settings: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1478009

Bisection shown that this issue fixed it.
Did you want to request uplift to beta for this change?
Flags: needinfo?(lgreco)
This patch is a reduced version of the patch we landed in nightly, and adapted to be applied on Firefox 62 beta.

This version of the patch selects the storage.local backend on startup only for the JSONFile backend (which is the one used by default in beta).

It fixes the issue with calling storage.local.set from a destroying context when the JSONFile backend is being used (and the test case is also adapted to test this behavior only with the JSONFile backend).
Attachment #8998206 - Flags: review?(aswan)
(In reply to Julien Cristau [:jcristau] from comment #27)
> Did you want to request uplift to beta for this change?

Yes, but we need to change the patch to be applied to beta, I just attached the adapted patch and set the r? flag on it.
Flags: needinfo?(lgreco)
Attachment #8998206 - Flags: review?(aswan) → review+
Comment on attachment 8998206 [details] [diff] [review]
fx62beta-Bug_1477015___Select_the_storage_local_backend_during_the_extension_startup_when_ExtensionStorageIDB_is_disabled_.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1406181
  
[User impact if declined]:
Some extensions are calling the storage.local API methods from extension contexts that are destroyed right after the API call (e.g. an extension popup which is closed immediately and/or an extension page that is navigated right after calling the storage.local API method), if the called storage.local API method is not a storage.local.get, the extension data is supposed to be updated accordingly and the storage.onChanged event to be fired.

Part of the changes introduced by Bug 1406181 have introduced a regression in this behavior also on the JSONFile backend (the default enabled on beta), which has been confirmed by QA when we have verified the fix we landed in Nightly (see comment 25), and this regression affects some of the existing extensions (e.g. uBlock Origin, as mentioned in comment 26).

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, the STR from comment 24 (without changing the "extensions.webextensions.ExtensionStorageIDB.enabled" preference, which in beta is already set to false by default, because the version of the patch we are uplifting to beta only covers the fix needed on the JSONFile backend).

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
The change should be low risky:
- the fix has been reduced to narrow its impact and to only cover the default storage.local backend enabled in beta (the JSONFile backend)
- it can only impact the extensions (and only the ones that are requesting the storage permission and use the storage.local API)
- additional automated tests has been added to the patch to cover these changes.

[String changes made/needed]:
None
Attachment #8998206 - Flags: approval-mozilla-beta?
Comment on attachment 8998206 [details] [diff] [review]
fx62beta-Bug_1477015___Select_the_storage_local_backend_during_the_extension_startup_when_ExtensionStorageIDB_is_disabled_.patch

May be a bit risky (for some extensions), but we still have several weeks left in beta, and this was verified in nightly and adds extra test coverage. Let's uplift for beta 17.
Attachment #8998206 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This issue is verified as fixed on Firefox 62.0b16 (20180809104529) under Win 7 64-bit and Mac OS X 10.13.2.

I will attach a postfix video.
Attached image Postfix video Beta — 
Depends on: 1492963

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: