Closed Bug 1445326 Opened 7 years ago Closed 7 years ago

Remove storage: "persistent" from indexed-db.js

Categories

(DevTools :: General, enhancement, P1)

Product:
DevTools
Component:
General
Version:
60 Branch
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

Details

(Whiteboard: [storage-v2])

Attachments

(2 files)

Bug 1445326 - Part 1 - Check for internal origins when assigning persistent indexedDB storage.
59 bytes, text/x-review-board-request
asuth
: review+
Details
Bug 1445326 - Part 2 - Remove the proprietary "storage" option from indexedDB.open in indexed-db.js.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
As part of deprecating the storage option for indexedDB.open(), we would like to remove the usage of storage: "persistent" in indexed-db.js (https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/shared/indexed-db.js#34).

In order to prevent data loss, we probably need to extend the check in https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/dom/indexedDB/IDBFactory.cpp#681 to cover the devtools principal like we do here: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/dom/quota/ActorsParent.cpp#5524

Ryan, what's the easiest way I can test the functionality of that database to verify I'm not losing any data?

Thanks!
Flags: needinfo?(jryans)
(In reply to Johann Hofmann [:johannh] from comment #0)
> Ryan, what's the easiest way I can test the functionality of that database
> to verify I'm not losing any data?

This DevTools DB is used for several UI features via the `async-storage` module[1].

If you mean manual testing, probably trying the Console history feature is the easiest:

1. Open the Console on some page (Tools -> Web Dev -> Console or Cmd-Opt-K / Ctrl-Shift-K)
2. Type some expression, press Enter to execute
3. Pressing Up Arrow in the Console input should recall the previous expression
4. Restart Firefox
5. Open the Console again
6. Pressing Up Arrow should still recall the same expression

If you mean automated testing, I am not sure the persistence of the DB is covered by DevTools tests today.

[1]: https://searchfox.org/mozilla-central/search?q=async-storage&case=true&path=
Flags: needinfo?(jryans)
Yup, I mostly meant manual testing. I don't think this is a case we can cover with automated tests. Thanks for the instructions!
Ryan, I verified in manual testing that this preserves the console state through restarts before and after the patch. Can you please check this again as well?

Andrew, I'm not sure whether this is the best way to get the principal here (aPrincipal doesn't seem to be present in most cases). I'm also unsure whether and how this should be tested. So I'm happy to take suggestions in these regards :)

Thank you both!
Comment on attachment 8959081 [details]
Bug 1445326 - Part 2 - Remove the proprietary "storage" option from indexedDB.open in indexed-db.js.

https://reviewboard.mozilla.org/r/227968/#review233912

Thanks, this change looks good to me! :)
Attachment #8959081 - Flags: review?(jryans) → review+
Whiteboard: [storage-v2][triage] → [storage-v2]
Comment on attachment 8959080 [details]
Bug 1445326 - Part 1 - Check for internal origins when assigning persistent indexedDB storage.

https://reviewboard.mozilla.org/r/227966/#review234902

::: dom/indexedDB/IDBFactory.cpp:691
(Diff revision 2)
>    }
>  
>    PersistenceType persistenceType;
>  
> -  if (principalInfo.type() == PrincipalInfo::TSystemPrincipalInfo) {
> -    // Chrome privilege always gets persistent storage.
> +  bool originIsInternal = false;
> +  if (aPrincipal) {

As you mention in comment 5, using aPrincipal is insufficient here because it doesn't cover worker cases and it would be an unpleasant surprise for devtools code if their main-thread/worker globals had separate storage partitions.

I think you want the if case to look like the following and forget about the aPrincipal check.

```
if (principalInfo.type() == PrincipalInfo::TSystemPrincipalInfo ||
    (principalInfo.type() == PrincipalInfo::TContentPrincipalInfo &&
     QuotaManager::IsOriginInternal(principalInfo.get_ContentPrincipalInfo().originNoSuffix())) {
    // Chrome and internal origins always gets persistent storage.
    persistenceType = PERSISTENCE_TYPE_PERSISTENT;
} else {
  persistenceType = PersistenceTypeFromStorage(aStorageType);
}
```

r=asuth with that change or something very close to it.  (Mainly, make sure if you extract the IsOriginInternal call, that you bring the tagged union check with it.)
Attachment #8959080 - Flags: review?(bugmail) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca569b4e9493
Part 1 - Check for internal origins when assigning persistent indexedDB storage. r=asuth
https://hg.mozilla.org/integration/autoland/rev/53ce4c598297
Part 2 - Remove the proprietary "storage" option from indexedDB.open in indexed-db.js. r=jryans
https://hg.mozilla.org/mozilla-central/rev/ca569b4e9493
https://hg.mozilla.org/mozilla-central/rev/53ce4c598297
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: