Opened 2 months ago

Closed 7 weeks ago

#31396 closed defect (fixed)

Communication with noscript for security settings not working in nightlies

Reported by: acat Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff68-esr, TorBrowserTeam201908R, tbb-9.0-must-alpha
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor44-can

Description

In current nightlies, changing security level does not modify NoScript settings. However, I verified that uninstalling the default NoScript, restarting the browser and installing NoScript from mozilla's addons page fixes this.

Child Tickets

Change History (9)

comment:1 Changed 2 months ago by gk

Keywords: TorBrowserTeam201908 tbb-9.0-must-alpha added

comment:2 Changed 8 weeks ago by pili

Sponsor: Sponsor44-can

Tagging with Sponsor 44

comment:3 Changed 7 weeks ago by acat

Status: newneeds_information

The issue has to do with a indexedDB error (InvalidStateError: A mutation operation was attempted on a database that did not allow mutations.), which is thrown when noscript tries to store something via browser.storage.local API (when it receives the setting change message from torbutton). This API moved to a indexedDB implementation in 62.

I tracked down the cause of indexedDB not working to the quota manager service clearing (Services.qms.clear()) in torbutton. This is done on startup and on New Identity.

I can reproduce this also on Firefox 68:

  1. Create a new profile.
  2. Install noscript from AMO.
  3. Set dom.quotaManager.testing pref to true.
  4. Call Services.qms.clear() in console.
  5. Set dom.quotaManager.testing pref to false.
  6. Open debug addon window for noscript in about:debugging.
  7. Run browser.storage.local.set({foo:bar}), it should throw InvalidStateError: A mutation operation was attempted on a database that did not allow mutations..

I think this does not affect indexeddb itself, but just the browser.storage.* implementation that uses indexeddb internally. I suspect Services.qms.clear() must be clearing some internal indexedDB data that is preventing the webextension storage API from working properly.

Not sure if this is really a bug from Firefox side, since probably we should not be calling Services.qms.clear() directly. But perhaps we can file one.

So we can solve this by not calling Services.qms.clear() (do we still need it? was it just for asm.js?). Or we can set extensions.webextensions.ExtensionStorageIDB.enabled to false, to use the previous non-indexedDB backend for browser.storage.*.

comment:4 in reply to:  3 Changed 7 weeks ago by gk

Replying to acat:

The issue has to do with a indexedDB error (InvalidStateError: A mutation operation was attempted on a database that did not allow mutations.), which is thrown when noscript tries to store something via browser.storage.local API (when it receives the setting change message from torbutton). This API moved to a indexedDB implementation in 62.

I tracked down the cause of indexedDB not working to the quota manager service clearing (Services.qms.clear()) in torbutton. This is done on startup and on New Identity.

I can reproduce this also on Firefox 68:

  1. Create a new profile.
  2. Install noscript from AMO.
  3. Set dom.quotaManager.testing pref to true.
  4. Call Services.qms.clear() in console.
  5. Set dom.quotaManager.testing pref to false.
  6. Open debug addon window for noscript in about:debugging.
  7. Run browser.storage.local.set({foo:bar}), it should throw InvalidStateError: A mutation operation was attempted on a database that did not allow mutations..

I think this does not affect indexeddb itself, but just the browser.storage.* implementation that uses indexeddb internally. I suspect Services.qms.clear() must be clearing some internal indexedDB data that is preventing the webextension storage API from working properly.

Not sure if this is really a bug from Firefox side, since probably we should not be calling Services.qms.clear() directly. But perhaps we can file one.

Please do.

So we can solve this by not calling Services.qms.clear() (do we still need it? was it just for asm.js?). Or we can set extensions.webextensions.ExtensionStorageIDB.enabled to false, to use the previous non-indexedDB backend for browser.storage.*.

Good questions. I don't know the answers without looking closer. Maybe we could file a ticket for that? Sounds like we should go with the other option for now. What are the drawbacks for that one?

comment:5 Changed 7 weeks ago by cypherpunks

In-memory IndexedDB is not ready. In-PBM too. Of course, extensions.webextensions.ExtensionStorageIDB.enabled for now.

comment:6 Changed 7 weeks ago by acat

In-memory IndexedDB is not ready. In-PBM too. Of course, extensions.webextensions.ExtensionStorageIDB.enabled for now.

I don't think in-memory indexeddb will be used in webextensions, it will always persist.

Sounds like we should go with the other option for now. What are the drawbacks for that one?

AFAIK the old implementation is synchronous and writes the whole key-value store to disk every time there is some change. But for reasonable usage of the storage (which noscript and https everywhere probably has) this should not be a problem.

There is some migration code that is ran when extensions.webextensions.ExtensionStorageIDB.enabled which might be removed in the future, but I think that should not be an issue at least until next ESR.

comment:7 Changed 7 weeks ago by acat

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201908 removed
Status: needs_informationneeds_review

comment:8 Changed 7 weeks ago by gk

Looks good to me. Thanks for digging down into that hole. I cherry-picked your patch onto tor-browser-68.1.0esr-9.0-1 (commit 1d8ad2218d07aa58e1887cb238499d08b70c190d).

Could you file the bugzilla ticket and mention it here? Feel free to close the bug after that.

comment:9 Changed 7 weeks ago by acat

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.