Issue 29760565: Issue 6599 - Detect data corruption of storage.local (Closed)
|
Created:
April 24, 2018, 11:02 a.m. by Sebastian Noack Modified:
April 27, 2018, 5:22 p.m. Visibility:
Public. |
DescriptionIssue 6599 - Detect data corruption of storage.local
Patch Set 1 #
Total comments: 3
Patch Set 2 : Detect when writing to storage.local fails but reading succeed #Patch Set 3 : Removed redundant Promise wrapper #
Total comments: 14
Patch Set 4 : Rebased #Patch Set 5 : Added comments, show uninstall page also when data corruption occurred #Patch Set 6 : Added dataCorrupted parameter to uninstall page #
Total comments: 9
Patch Set 7 : Rebased #Patch Set 8 : Fixed typos in comment #Patch Set 9 : Made reinitialized and dataCorrupted a getter #Patch Set 10 : Updated adblockplusui #Patch Set 11 : Fixed circular dependency #
Total comments: 26
Patch Set 12 : Added comment, fixed typo #Patch Set 13 : Added missing semicolon, made customSave logic respond asynchrnously #
Total comments: 2
Patch Set 14 : Fixed blocked_total optimization logic #
MessagesTotal messages: 27
I guess to test this we could override parts of the chrome.storage API to throw an exception, then see what happens. https://codereview.adblockplus.org/29760565/diff/29760566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29760566/lib/subscriptionIni... lib/subscriptionInit.js:244: if (firstRun || exports.dataCorrupted) Well surely this would mean that the first run page constantly opens instead of the updates page for these users with corrupted data? Shouldn't we show a page which explains that their data is corrupted and that they should try reinstalling the extension? Also I wonder if there's another way to store a flag to somehow avoid constantly showing the page (or if that's a good idea)?
On 2018/04/24 11:18:01, kzar wrote: > I guess to test this we could override parts of the chrome.storage API to throw > an exception, then see what happens. Yeah, something like that. https://codereview.adblockplus.org/29760565/diff/29760566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29760566/lib/subscriptionIni... lib/subscriptionInit.js:244: if (firstRun || exports.dataCorrupted) On 2018/04/24 11:18:01, kzar wrote: > Well surely this would mean that the first run page constantly opens instead of > the updates page for these users with corrupted data? > > Shouldn't we show a page which explains that their data is corrupted and that > they should try reinstalling the extension? Also I wonder if there's another way > to store a flag to somehow avoid constantly showing the page (or if that's a > good idea)? Yes, that is the idea, to add this message to the first run page, but hide it unless dataCorrupted is set. Note that this how we already handle the "reinitialized" flag.
Otherwise the approach and changes themselves LGTM https://codereview.adblockplus.org/29760565/diff/29760566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29760566/lib/subscriptionIni... lib/subscriptionInit.js:244: if (firstRun || exports.dataCorrupted) On 2018/04/24 11:26:26, Sebastian Noack wrote: > Yes, that is the idea, to add this message to the first run page, > but hide it unless dataCorrupted is set. Note that this how we already > handle the "reinitialized" flag. Fair enough, I guess we need to also update the adblockplusui dependency to include that with along with this change.
On 2018/04/24 11:18:01, kzar wrote:
> I guess to test this we could override parts of the chrome.storage API to
throw
> an exception, then see what happens.
When I locally patch Adblock Plus to make `storage.local.{get|set}` always fail
(asynchronously), in order to emulate the effect of the data corruption, I
always get the firstRun page already (which some users have seen as well).
However, the only way in which the updates page would currently keep showing is
if only storage.local.set() fails while the get() succeeds. I changed the
approach to detect this scenario as well.
I think the approach looks pretty good, handling both if the preferences fail to load initially and if setting the updateVersion fails. But I wonder if we should also attempt saving a preference for existing Firefox users so that we can catch the problem in time to display the error message for them too? https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js File lib/prefs.js (left): https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#oldcod... lib/prefs.js:264: ext.storage.set(prefToKey(pref), overrides[pref]); As discussed in IRC let's replace ext.storage with browser.storage everywhere in another patch set, to keep make this one easier to understand. https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:234: return browser.storage.local.remove(prefToKey(preference)); Nit: Since we're just calling browser.storage.local.remove directly here along with prefToKey it seems kinda inconsistent that we're then using this special savePref function for saving which does that for us. https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:342: let localLoaded = browser.storage.local.get(prefs.map(prefToKey)).then(items => Nit: This line is too long in Rietveld, did it pass ESLint? https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:345: overrides[keyToPref(key)] = items[key]; We just had the key above, so is there not a way we can remember it to avoid looking it up again here? https://codereview.adblockplus.org/29760565/diff/29760664/lib/subscriptionIni... File lib/subscriptionInit.js (left): https://codereview.adblockplus.org/29760565/diff/29760664/lib/subscriptionIni... lib/subscriptionInit.js:248: // For now we're limiting the updates page to users of Mind adding some comments back in? I don't mind if they're less verbose but IMO it's useful to briefly explain the behaviour of this code - when each page is expected to be displayed.
https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js File lib/prefs.js (left): https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#oldcod... lib/prefs.js:264: ext.storage.set(prefToKey(pref), overrides[pref]); On 2018/04/24 16:18:46, kzar wrote: > As discussed in IRC let's replace ext.storage with browser.storage everywhere in > another patch set, to keep make this one easier to understand. There we go: https://codereview.adblockplus.org/29760680 I now rebased this patch on top of that change. https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:234: return browser.storage.local.remove(prefToKey(preference)); On 2018/04/24 16:18:46, kzar wrote: > Nit: Since we're just calling browser.storage.local.remove directly here along > with prefToKey it seems kinda inconsistent that we're then using this special > savePref function for saving which does that for us. Well, nothing changed in this regard. Except for returning a promise, the code in this function is identical to the code we had in the setter before. Also note that savePref() is also called from the customSave callback, so if we want to get rid of it we also have to inline the code there. Plus, the one liner below will become a little more complex (since we cannot just use || then anymore). https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:342: let localLoaded = browser.storage.local.get(prefs.map(prefToKey)).then(items => On 2018/04/24 16:18:46, kzar wrote: > Nit: This line is too long in Rietveld, did it pass ESLint? This is no longer the case after the rebase. https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:345: overrides[keyToPref(key)] = items[key]; On 2018/04/24 16:18:46, kzar wrote: > We just had the key above, so is there not a way we can remember it to avoid > looking it up again here? What do you have in mind? let keys = prefs.map(prefToKey); let localLoaded = browser.storage.local.get(keys).then(items => { for (let key of items) overrides[prefs[keys.indexOf(key)]] = items[key]; }); Hardly any more readable (or faster) than what we are currently doing. https://codereview.adblockplus.org/29760565/diff/29760664/lib/subscriptionIni... File lib/subscriptionInit.js (left): https://codereview.adblockplus.org/29760565/diff/29760664/lib/subscriptionIni... lib/subscriptionInit.js:248: // For now we're limiting the updates page to users of On 2018/04/24 16:18:46, kzar wrote: > Mind adding some comments back in? I don't mind if they're less verbose but IMO > it's useful to briefly explain the behaviour of this code - when each page is > expected to be displayed. OK, I added some more meaningful comments.
On 2018/04/24 16:18:46, kzar wrote: > But I wonder if we should > also attempt saving a preference for existing Firefox users so that we can catch > the problem in time to display the error message for them too? So far it's not even certain that Firefox has the same issue, specifically reading from storage.local succeeds, but writing to it fails. If reading from storage.local fails on Firefox this is already handled. But I tend to not make the logic even more complex to account for a hypothetical scenario.
LGTM once the adblockplusui changes are included too. > So far it's not even certain that Firefox has the same > issue, specifically reading from storage.local succeeds, > but writing to it fails. If reading from storage.local > fails on Firefox this is already handled. But I tend to > not make the logic even more complex to account for a > hypothetical scenario. Fair enough. https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:342: let localLoaded = browser.storage.local.get(prefs.map(prefToKey)).then(items => On 2018/04/24 17:49:25, Sebastian Noack wrote: > On 2018/04/24 16:18:46, kzar wrote: > > Nit: This line is too long in Rietveld, did it pass ESLint? > > This is no longer the case after the rebase. Acknowledged. https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:345: overrides[keyToPref(key)] = items[key]; On 2018/04/24 17:49:25, Sebastian Noack wrote: > On 2018/04/24 16:18:46, kzar wrote: > > We just had the key above, so is there not a way we can remember it to avoid > > looking it up again here? > > What do you have in mind? > > let keys = prefs.map(prefToKey); > let localLoaded = browser.storage.local.get(keys).then(items => > { > for (let key of items) > overrides[prefs[keys.indexOf(key)]] = items[key]; > }); > > Hardly any more readable (or faster) than what we are currently doing. Yea, fair enough I can't see a much better way to do it either. https://codereview.adblockplus.org/29760565/diff/29760664/lib/subscriptionIni... File lib/subscriptionInit.js (left): https://codereview.adblockplus.org/29760565/diff/29760664/lib/subscriptionIni... lib/subscriptionInit.js:248: // For now we're limiting the updates page to users of On 2018/04/24 17:49:25, Sebastian Noack wrote: > On 2018/04/24 16:18:46, kzar wrote: > > Mind adding some comments back in? I don't mind if they're less verbose but > IMO > > it's useful to briefly explain the behaviour of this code - when each page is > > expected to be displayed. > > OK, I added some more meaningful comments. Thanks
https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29760664/lib/prefs.js#newcod... lib/prefs.js:345: overrides[keyToPref(key)] = items[key]; On 2018/04/25 10:53:51, kzar wrote: > Yea, fair enough I can't see a much better way to do it either. (I had assumed that browser.storage.local.get would return an Array of values if you requested an Array of values, but it returns them in an Object.)
As you suggested earlier, and data now approved, I added a dataCorrupted parameter to the uninstall page. Please review again. Thanks.
https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... lib/subscriptionInit.js:293: exports.dataCorrupted = false; I just realised that this should probably be a getter instead. Otherwise if it's required directly the value won't update and that might not be obvious. https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js#ne... lib/uninstall.js:23: const subscriptionInit = require("./subscriptionInit.js"); Nit: const {dataCorrupted} = require(...)? https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js#ne... lib/uninstall.js:31: * Must be called after prefs got initilized and a data corruption Nit: A couple typos 'initialized' and 'notification'.
https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... lib/subscriptionInit.js:293: exports.dataCorrupted = false; On 2018/04/25 14:38:12, kzar wrote: > I just realised that this should probably be a getter instead. Otherwise if it's > required directly the value won't update and that might not be obvious. Doesn't strike me as a huge issue. But more importantly it is consistent with "reinitialized", and I'd rather not make this a getter without making "reinitialized" a getter as well, but then we are going down a rabbit hole of unrelated changes (also considering adblockplusui that relies on those). https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js#ne... lib/uninstall.js:23: const subscriptionInit = require("./subscriptionInit.js"); On 2018/04/25 14:38:12, kzar wrote: > Nit: const {dataCorrupted} = require(...)? At the time we import subscriptionInit.js, dataCorrupted might not be set to its final value yet. https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js#ne... lib/uninstall.js:31: * Must be called after prefs got initilized and a data corruption On 2018/04/25 14:38:12, kzar wrote: > Nit: A couple typos 'initialized' and 'notification'. Done.
https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... lib/subscriptionInit.js:293: exports.dataCorrupted = false; On 2018/04/25 14:46:23, Sebastian Noack wrote: > On 2018/04/25 14:38:12, kzar wrote: > > I just realised that this should probably be a getter instead. Otherwise if > it's > > required directly the value won't update and that might not be obvious. > > Doesn't strike me as a huge issue. But more importantly it is consistent with > "reinitialized", and I'd rather not make this a getter without making > "reinitialized" a getter as well, but then we are going down a rabbit hole of > unrelated changes (also considering adblockplusui that relies on those). Well I agree about not randomly changing "reinitialized" here, but not about not making "dataCorrupted" a getter. To my mind "dataCorrupted" could change when we detect that the data has been corrupted, even if we don't have such a check implemented yet. Your example of not wanting to change code already used elsewhere to / from a getter is a good example why we should make this a getter now. https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29760565/diff/29761558/lib/uninstall.js#ne... lib/uninstall.js:23: const subscriptionInit = require("./subscriptionInit.js"); On 2018/04/25 14:46:23, Sebastian Noack wrote: > On 2018/04/25 14:38:12, kzar wrote: > > Nit: const {dataCorrupted} = require(...)? > > At the time we import subscriptionInit.js, dataCorrupted might not be set to its > final value yet. That's why I suggest making it a getter instead.
https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29761558/lib/subscriptionIni... lib/subscriptionInit.js:293: exports.dataCorrupted = false; On 2018/04/25 14:57:04, kzar wrote: > On 2018/04/25 14:46:23, Sebastian Noack wrote: > > On 2018/04/25 14:38:12, kzar wrote: > > > I just realised that this should probably be a getter instead. Otherwise if > > it's > > > required directly the value won't update and that might not be obvious. > > > > Doesn't strike me as a huge issue. But more importantly it is consistent with > > "reinitialized", and I'd rather not make this a getter without making > > "reinitialized" a getter as well, but then we are going down a rabbit hole of > > unrelated changes (also considering adblockplusui that relies on those). > > Well I agree about not randomly changing "reinitialized" here, but not about not > making "dataCorrupted" a getter. To my mind "dataCorrupted" could change when we > detect that the data has been corrupted, even if we don't have such a check > implemented yet. Your example of not wanting to change code already used > elsewhere to / from a getter is a good example why we should make this a getter > now. I disagree about only changing either of them. So I turned both now into a getter. I added Thomas CC, so that he knows that he has to account for those changes in adblockplusui. FWIW, I couldn't test the latest patch set yet, will do once there is a version of adblockplusui, I can test it with.
Thanks, LGTM once the adblockplusui changes are in.
On 2018/04/25 17:17:31, kzar wrote: > Thanks, LGTM once the adblockplusui changes are in. FYI: The UI change has landed: https://hg.adblockplus.org/adblockplusui/rev/8c49749e63e7
On 2018/04/26 16:10:49, Thomas Greiner wrote: > FYI: The UI change has landed: > https://hg.adblockplus.org/adblockplusui/rev/8c49749e63e7 Thanks! I updated the adblockplusui dependency and did some brief testing. The message shows up and looks good, if I patched the storage.local API to always fail.
https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:277: .then(() => { require("./uninstall").setUninstallURL() }) I found an issue with the circular dependency of the uninstall and subscriptionInit modules. Without this change, I get "isDataCorrupted is not a function" in the background page log, causing the uninstall page to not show.
Mostly suggestions and promise-related comments. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:229: throw new Error("Attempt to change preference type"); Why don't we return `Promise.reject()` here? It's quite unexpected to have to catch errors in two different ways when expecting a promise. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:319: callback(); What if an error occurs in `savePref()`? I'd argue we'd want to catch that so returning the promise from `savePref()` here, instead of `Promise.resolve()` would be helpful. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:329: set(value) Suggestion: What about marking this setter as deprecated to indicate that any new code should be using `Prefs.set()`? https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:250: return Prefs.set("last_updates_page_displayed", updatesVersion).catch(() => Personally, I'd love to do that check before running any of our initialization code but I understand that this would require further discussion so for the sake of the hotfix feel free to ignore this comment. https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:258: // (either though failure of reading from or writing to storage.local). Typo: Replace "though" with "through". https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:277: .then(() => { require("./uninstall").setUninstallURL() }) On 2018/04/26 18:25:35, Sebastian Noack wrote: > I found an issue with the circular dependency of the uninstall and > subscriptionInit modules. Without this change, I get "isDataCorrupted is not a > function" in the background page log, causing the uninstall page to not show. Acknowledged. Mind adding an inline comment for that? https://codereview.adblockplus.org/29760565/diff/29762702/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/uninstall.js#ne... lib/uninstall.js:58: search.push("dataCorrupted=" + (isDataCorrupted() ? "1" : "0")); Suggestion: Instead of passing this parameter on each download even for those who don't even encounter this issue, what about an "issue" query string parameter which can specify one or more issues? (e.g. "?issue=dataCorrupted&issues=reinitialized&...") On the server-side we should be able to infer using "addonVersion" that if the parameter is missing that the issue wasn't encountered.
https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:229: throw new Error("Attempt to change preference type"); On 2018/04/27 14:50:12, Thomas Greiner wrote: > Why don't we return `Promise.reject()` here? It's quite unexpected to have to > catch errors in two different ways when expecting a promise. Well, if we already know immediately/synchronously about an error why signaling it later? Also this isn't an error that would ever need to be caught, but its more like an assertion that indicates a bug, if this error is ever thrown. Furthermore, if using `Prefs.foo = ...` this function is called implicitly and the promise won't be accessible. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:319: callback(); On 2018/04/27 14:50:12, Thomas Greiner wrote: > What if an error occurs in `savePref()`? I'd argue we'd want to catch that so > returning the promise from `savePref()` here, instead of `Promise.resolve()` > would be helpful. Yes, returning a resolved promise immediately is a hack, so is this whole workaround here. Returning a promise that resolves when the deferred storage operation completes, will make the code notably more complex, plus it might take up to a minute for the promise to resolve, which might have weird side effects if we'd ever rely on a promise returned here. Also note that this code path is essentially only used for the blocked_total preference and only on Firefox. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:329: set(value) On 2018/04/27 14:50:12, Thomas Greiner wrote: > Suggestion: What about marking this setter as deprecated to indicate that any > new code should be using `Prefs.set()`? We didn't decide yet, whether we want to consider using the setter properties deprecated, or rather only use the set() method in order to detect the data corruption during initialization. https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:250: return Prefs.set("last_updates_page_displayed", updatesVersion).catch(() => On 2018/04/27 14:50:13, Thomas Greiner wrote: > Personally, I'd love to do that check before running any of our initialization > code but I understand that this would require further discussion so for the sake > of the hotfix feel free to ignore this comment. I guess we could add a dummy preference, and try to read/write in order to detect the data corruption before the actual initialization, but this seems to make things more complicated, and potentially less robust since theoretically at least reading/writing the dummy preference might succeed while the IO operations here still might fail. https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:258: // (either though failure of reading from or writing to storage.local). On 2018/04/27 14:50:13, Thomas Greiner wrote: > Typo: Replace "though" with "through". Done. https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:277: .then(() => { require("./uninstall").setUninstallURL() }) On 2018/04/27 14:50:13, Thomas Greiner wrote: > On 2018/04/26 18:25:35, Sebastian Noack wrote: > > I found an issue with the circular dependency of the uninstall and > > subscriptionInit modules. Without this change, I get "isDataCorrupted is not a > > function" in the background page log, causing the uninstall page to not show. > > Acknowledged. Mind adding an inline comment for that? Done. https://codereview.adblockplus.org/29760565/diff/29762702/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/uninstall.js#ne... lib/uninstall.js:58: search.push("dataCorrupted=" + (isDataCorrupted() ? "1" : "0")); On 2018/04/27 14:50:13, Thomas Greiner wrote: > Suggestion: Instead of passing this parameter on each download even for those > who don't even encounter this issue, what about an "issue" query string > parameter which can specify one or more issues? (e.g. > "?issue=dataCorrupted&issues=reinitialized&...") > > On the server-side we should be able to infer using "addonVersion" that if the > parameter is missing that the issue wasn't encountered. This seems somewhat premature. I guess we can change this, once we start indicating different issues here (if ever).
https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:319: callback(); On 2018/04/27 16:01:04, Sebastian Noack wrote: > On 2018/04/27 14:50:12, Thomas Greiner wrote: > > What if an error occurs in `savePref()`? I'd argue we'd want to catch that so > > returning the promise from `savePref()` here, instead of `Promise.resolve()` > > would be helpful. > > Yes, returning a resolved promise immediately is a hack, so is this whole > workaround here. Returning a promise that resolves when the deferred storage > operation completes, will make the code notably more complex, plus it might take > up to a minute for the promise to resolve, which might have weird side effects > if we'd ever rely on a promise returned here. Also note that this code path is > essentially only used for the blocked_total preference and only on Firefox. Never mind, it turned out returning asynchronously here is simple enough. Done.
Only one more comment to tackle and we'll be good to go. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:229: throw new Error("Attempt to change preference type"); On 2018/04/27 16:01:03, Sebastian Noack wrote: > Well, if we already know immediately/synchronously about an error why signaling > it later? Also this isn't an error that would ever need to be caught, but its > more like an assertion that indicates a bug, if this error is ever thrown. > Furthermore, if using `Prefs.foo = ...` this function is called implicitly and > the promise won't be accessible. True, if that's a requirement we could check twice. Once before calling `Prefs.set()` to throw the error and once inside `Prefs.set()` to return `Promise.reject()`. That way we won't affect any code that already uses `Prefs.set()` when we drop the setter. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:319: callback(); On 2018/04/27 16:14:52, Sebastian Noack wrote: > Never mind, it turned out returning asynchronously here is simple enough. Done. Thanks. https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:329: set(value) On 2018/04/27 16:01:03, Sebastian Noack wrote: > On 2018/04/27 14:50:12, Thomas Greiner wrote: > > Suggestion: What about marking this setter as deprecated to indicate that any > > new code should be using `Prefs.set()`? > > We didn't decide yet, whether we want to consider using the setter properties > deprecated, or rather only use the set() method in order to detect the data > corruption during initialization. Acknowledged. https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:250: return Prefs.set("last_updates_page_displayed", updatesVersion).catch(() => On 2018/04/27 16:01:04, Sebastian Noack wrote: > I guess we could add a dummy preference, and try to read/write in order to > detect the data corruption before the actual initialization, but this seems to > make things more complicated, and potentially less robust since theoretically at > least reading/writing the dummy preference might succeed while the IO operations > here still might fail. Agreed that more thought needs to be put into it so if you agree I'll create a separate ticket for that for later. https://codereview.adblockplus.org/29760565/diff/29762702/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/uninstall.js#ne... lib/uninstall.js:58: search.push("dataCorrupted=" + (isDataCorrupted() ? "1" : "0")); On 2018/04/27 16:01:04, Sebastian Noack wrote: > This seems somewhat premature. I guess we can change this, once we start > indicating different issues here (if ever). Ok, I'll bring this up with the Data team for tackling it afterwards.
https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:229: throw new Error("Attempt to change preference type"); On 2018/04/27 16:27:33, Thomas Greiner wrote: > On 2018/04/27 16:01:03, Sebastian Noack wrote: > > Well, if we already know immediately/synchronously about an error why > signaling > > it later? Also this isn't an error that would ever need to be caught, but its > > more like an assertion that indicates a bug, if this error is ever thrown. > > Furthermore, if using `Prefs.foo = ...` this function is called implicitly and > > the promise won't be accessible. > > True, if that's a requirement we could check twice. Once before calling > `Prefs.set()` to throw the error and once inside `Prefs.set()` to return > `Promise.reject()`. That way we won't affect any code that already uses > `Prefs.set()` when we drop the setter. Well, compatibility with the setter property isn't the only reason to throw synchronously here, as explained in my comment above. https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/subscriptionIni... lib/subscriptionInit.js:250: return Prefs.set("last_updates_page_displayed", updatesVersion).catch(() => On 2018/04/27 16:27:33, Thomas Greiner wrote: > On 2018/04/27 16:01:04, Sebastian Noack wrote: > > I guess we could add a dummy preference, and try to read/write in order to > > detect the data corruption before the actual initialization, but this seems to > > make things more complicated, and potentially less robust since theoretically > at > > least reading/writing the dummy preference might succeed while the IO > operations > > here still might fail. > > Agreed that more thought needs to be put into it so if you agree I'll create a > separate ticket for that for later. Well, if you have any specific idea that would account for my concerns, sure.
https://codereview.adblockplus.org/29760565/diff/29763867/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29763867/lib/prefs.js#newcod... lib/prefs.js:310: promise = null; Since callback() were potentially called synchronously below, this will potentially reset promise to null before it gets returned. I figured the simplest solution is to just always call setTimeout().
https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:229: throw new Error("Attempt to change preference type"); On 2018/04/27 16:33:22, Sebastian Noack wrote: > Well, compatibility with the setter property isn't the only reason to throw > synchronously here, as explained in my comment above. As I said, we can still throw the error in the setter while returning `Promise.reject()` here so nothing will change how the setter is currently used and it could therefore still fulfill the purpose of an assertion. Going forward, however, it would allow us to use `Prefs.set().catch()` to catch all errors, including this one, instead of having to put a try-catch around it as well. https://codereview.adblockplus.org/29760565/diff/29763867/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29763867/lib/prefs.js#newcod... lib/prefs.js:310: promise = null; On 2018/04/27 16:58:58, Sebastian Noack wrote: > Since callback() were potentially called synchronously below, this will > potentially reset promise to null before it gets returned. I figured the > simplest solution is to just always call setTimeout(). Acknowledged.
https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:229: throw new Error("Attempt to change preference type"); On 2018/04/27 17:03:32, Thomas Greiner wrote: > On 2018/04/27 16:33:22, Sebastian Noack wrote: > > Well, compatibility with the setter property isn't the only reason to throw > > synchronously here, as explained in my comment above. > > As I said, we can still throw the error in the setter while returning > `Promise.reject()` here so nothing will change how the setter is currently used > and it could therefore still fulfill the purpose of an assertion. > > Going forward, however, it would allow us to use `Prefs.set().catch()` to catch > all errors, including this one, instead of having to put a try-catch around it > as well. Yes, we could signal the error asynchronously here and redundantly check in the setter to signal it synchronously there, but this will involve some (undesired) duplication. And again, I think it is totally fine if any calling code just handles the promise rejection and ignores the error thrown here. It's just an assertion, to avoid accidentally assigning a value of the wrong type, during development. If this error is ever thrown, the code that triggers it should be changed rather than handling that error.
LGTM https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29760565/diff/29762702/lib/prefs.js#newcod... lib/prefs.js:229: throw new Error("Attempt to change preference type"); On 2018/04/27 17:11:11, Sebastian Noack wrote: > Yes, we could signal the error asynchronously here and redundantly check in the > setter to signal it synchronously there, but this will involve some (undesired) > duplication. And again, I think it is totally fine if any calling code just > handles the promise rejection and ignores the error thrown here. It's just an > assertion, to avoid accidentally assigning a value of the wrong type, during > development. If this error is ever thrown, the code that triggers it should be > changed rather than handling that error. Note that assertions may fail asynchronously in which case such errors could occur not only during development but also in production. So while I'm not happy about ignoring errors, I can understand it since that appears to be a convention here. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
