[Feature Request] Better settings management and internal data separation

[mwsundberg]

Is your feature request related to a problem? Please describe.
Currently there are 3 settings values that are stored, blocking_enabled (renamed in #42 from prior key state (EDIT: rename potentially going to be reverted)), notificationsAllowed, and allowed_domain_list. These values are stored in the extension's storage.local StorageArea, along with blocked_hosts, blocked_ports, and badgeswhich all hold logging data of blocked hosts/ports/counts on a per-tab basis.
Additionally, settings aren't set to their default values in one central place, instead default values are passed at each storage-reading function call, leading to potentially confused state if different defaults are used in different checks.

Describe the solution you'd like

• Have a settings migration and defaults initialization method that is called on extension install or optionally by a "reset settings" button on the options page. Will also need to also account for not overriding users' existing whitelists/configs as this feature gets rolled out and on subsequent extension updates.
  • Implement versioning for stored settings data to make schema upgrades easier. Probably way over-engineering it but could also be useful. Likely won't work on this unless it's deemed necessary
• Drop unlimitedStorage from the extension's permissions. If the extension is somehow using more storage than the cap allows, it's probably a memory/storage leak somewhere (which would be harder to catch since unlimitedStorage would disable any storage overuse errors by raising the cap unreachably high)
• Reorganize all per-tab data (badges, blocked_hosts, blocked_ports, and anything new added)
  • Move to the storage.session StorageArea to be cleared on browser-close. Pretty sure this isn't necessary from a memory-leak perspective (background.js:handleUpdated() I think cleans up well enough on tab navigation), but it's a lot of data sitting around in permanent storage that doesn't need to be.
  • Flip access pattern from <data_type>[tab_id] to tab_data[tab_id].<data_type> or [tab_id].<data_type> (stored "naked" under key tab_id. Would enable safer parallel read/writes (after BrowserStorageManager changes to enable) but could more easily lose data).
• Stop JSON.stringify()ing all data before storing. MDN says it's safe to store basic objects, arrays, etc. directly. (EDIT: postponed, might have issues in our code around shallow/deep object copying or stale references, want to investigate further)

Additional context
Mainly just tossing some ideas I planned to work on into the public forum to get feedback on. When I get to it I'll have a new branch on my fork if people want to poke and prod at the code.

EDIT: Moved settings sync to own issue: #49

[ACK-J]

Good call on dropping unlimitedStorage, I think I added that back when there was a memory leak issue in Port Authority and was having trouble fixing it. It should be safe to take that out.

Session storage does seem like a better location for badges, blocked_hosts, blocked_ports since if a browser is closed and then re-opened, the pages are refreshed anyways which would re-populate the blocked requests. I've been meaning to add a deleteItemFromLocal() or similar function to call when a tab is closed to be more memory efficient.

Storage.sync would be really convenient to sync settings for heavy Mozilla users but I'd suggest we keep it off by default.

[mwsundberg]

Added a commit to the current PR dropping unlimitedStorage but holding off on storage.session migration or deleteItemFromLocal() as to give you time to review without the code changing under your nose again and again.

Sounds good on storage.sync being off by default.

Just had an idea, what do you think about restructuring badges, blocked_hosts, blocked_ports during or after the move to session storage? Current access is like <data_type>[tab_id], storing it flipped like tab_data[tab_id].<data_type> (or "naked" keyed in storage directly under tab_id) would make for less "unwrapping" code and simplify clearing out tabs' data on close (one deleteItemFromLocal(tab_id) vs current background.js handleChange() which hits storage 3 times for each <data_type>).

That said, I'm not sure if the current <data_type>[tab_id] structure was chosen to address some other issue, I don't want to undo that if it is important somehow. Also changing it is fairly unimportant compared to speeding up blocking logic or removing Bootstrap, but I do think it could be a small positive tweak nonetheless.

// Current structure: <data_type>[tab_id]
"badges": {
  "2": {
    "counter": 54,
    "alerted": 1,
    "lastURL": "file:///.../TestPortScans.html"
  },
  "4": ...,
  "13": ...,
},

"blocked_hosts": {
  "2": [
    "fp.disney.go.com",
    "h.chase.co.uk",
    ...
  ],
  "4": ...,
  "13": ...,
},

"blocked_ports": {
  "2": {
    "127.0.0.1": ["8081"],
    "localhost": ["8080","443","433"],
    "10.0.0.1": ["455"],
    ...
  },
  "4": ...,
  "13": ...,
}

or

// Proposed structure: [tab_id].<data_type>
"2": {
  "badges": {
    "counter": 54,
    "alerted": 1,
    "lastURL": "file:///.../TestPortScans.html"
  },
  "blocked_hosts": [
    "fp.disney.go.com",
    "h.chase.co.uk",
    ...
  ],
  "blocked_ports": {
    "127.0.0.1": ["8081"],
    "localhost": ["8080","443","433"],
    "10.0.0.1": ["455"],
    ...
  }
},

"4": {
  "badges": ...,
  "blocked_hosts": ...,
  "blocked_ports": ...
},

"13": {
  "badges": ...,
  "blocked_hosts": ...,
  "blocked_ports": ...
}

[ACK-J]

The storage.session migration sounds good to me and makes sense based on our previous discussion, no hesitancy there!

Re: Restructuring
I like your proposed structure better to reduce the number of extension storage requests even though we already solved the race-condition nightmare. The proposed version is much more organized and easier to work with since the data is all interconnected. This will also make removing data on tab-close much simpler. The current structure was chosen out of simplicity without much thought going to efficiency.

[mwsundberg]

Re: Restructuring

Sounds good. Way down the line (not even in issue-making territory for me at least) I'd like to add parallel write support on a per-storage-key basis, but that'll require a bunch more work to get to and then a lot more to make sure deadlocks and such don't crop up. Flipping the data structure to be [tab_id]<data_type> does go a long ways towards making that possible, though!