Closed Bug 1321303 Opened 9 years ago Closed 9 years ago

Implement some of the browsingData.remove WebExtensions API methods

Categories

(WebExtensions :: Compatibility, defect, P2)

Product:
WebExtensions
Component:
Compatibility
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Iteration:
53.3 - Dec 26
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [browsingData]triaged)

Attachments

(8 files)

Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies
58 bytes, text/x-review-board-request
aswan
: review+
mak
: review+
Details
Bug 1321303 - Part 2: Implement browsingData.removeCache,
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1321303 - Part 3: Implement browsingData.removeHistory,
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1321303 - Part 4: Implement browsingData.removeDownloads,
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1321303 - Part 5: Implement browsingData.removeFormData,
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1321303 - Part 6: Implement browsingData.removePluginData,
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1321303 - Part 7: Implement browsingData.removePasswords
59 bytes, text/x-review-board-request
aswan
: review+
Dolske
: review+
Details
Bug 1321303 - Part 8: Implement browsingData serviceWorkers dataType,
59 bytes, text/x-review-board-request
baku
: review+
Details
This method allows the user to remove cookies. The docs can be found at [1]. In addition to this specific method, this bug will also cover creating the more generic remove method [2], but will only cover the cookies data type for that method. [1] https://developer.chrome.com/extensions/browsingData#method-removeCookies [2] https://developer.chrome.com/extensions/browsingData#method-remove
I think it makes sense to implement a few of the removal methods via this bug (i.e., not just cookies), so I am updating the bug and will add multiple patches to it, the first extra one being for cache.
Summary: Implement browsingData.removeCookies WebExtensions API method → Implement some of the browsingData.remove WebExtensions API methods
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review97246 Several medium-sized issues here, I'd like to take a look again. ::: browser/components/extensions/ext-browsingData.js:24 (Diff revision 4) > +let _sanitizer; > +function getSanitizer() { > + if (!_sanitizer) { > + _sanitizer = new Sanitizer(); > + _sanitizer.prefDomain = PREF_DOMAIN; > + } > + return _sanitizer; this could just be done with `XPCOMUtils.defineLazyGetter()` ::: browser/components/extensions/ext-browsingData.js:40 (Diff revision 4) > + null : > + [PlacesUtils.toPRTime(options.since), PlacesUtils.toPRTime(Date.now())]; > + return getSanitizer().items.cookies.clear(range); > +} > + > +function doRemoval(options, methodOrDataToRemove) { Overloading this to handle a bunch of different functions is unnecessarily complicated. How about making the validation of `options` its own function, then having `removeCookies()` call `clearCookies` directly. As a bonus, you could just inline the logic currently in `doRemove()` right into the implementation of `remove()` ::: browser/components/extensions/ext-browsingData.js:57 (Diff revision 4) > + // It's the generic remove method. > + return doRemove(options, methodOrDataToRemove); > +} > + > +function doRemove(options, dataToRemove) { > + // Check for unsupported data types. This is fragile since you need to keep your unsupported list here in sync with the schema. Also at the moment this lands, we won't fail if `remove()` is called with something that we intend to support but don't currently support (eg, cache, history, etc) How about iterating over the keys of dataToRemove instead, building your promises as necessary and rejecting if you encounter something unsupported. ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_cookies.js:52 (Diff revision 4) > + ok(!Services.cookies.cookieExists(COOKIE), `Cookie ${COOKIE.name} was removed.`); > + > + // Add a cookie which will end up with an older creationTime. > + let oldCookie = Object.assign({}, COOKIE, {name: Date.now()}); > + addCookie(oldCookie); > + await new Promise(resolve => setTimeout(resolve, 200)); why don't you add the creationTime as a parameter to `addCookie()` and back-date the cookies you're adding rather than waiting idly? ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_cookies.js:58 (Diff revision 4) > + > + // Add a cookie which will end up with a more recent creationTime. > + addCookie(COOKIE); > + > + // Clear cookies with a since value. > + let since = Date.now() - 100; This is racy and almost certainly will lead to intermittents. If you follow the suggestion above about passing explicit creationTimes to `addCookie()`, you can read the clock once and set this up correctly so you're not depending on things happening within a particular time frame.
Attachment #8816194 - Flags: review?(aswan) → review-
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review97246 > Overloading this to handle a bunch of different functions is unnecessarily complicated. How about making the validation of `options` its own function, then having `removeCookies()` call `clearCookies` directly. As a bonus, you could just inline the logic currently in `doRemove()` right into the implementation of `remove()` That is in fact exactly how I wrote it originally, but I then I ended up with repeated boilerplate code in each API method to call the `validateOptions` function and then reject if validation failed. I was trying to figure out a way so that I could centralize this code that checks options, and returns a rejected promise, and what I came up with was that the API methods themselves would simply return the result of another function, which itself could do the validation and return a rejected promise. I'm not crazy about my result either, but simply moving the validation into its own function doesn't solve the problem, from what I can tell anyway. What is a better way to do this so I don't need code like: ``` if (!validateOptions(options)) { return Promise.reject({message: "My validation failure message here"}); } ``` in every API method? I tried just throwing an exception in the validation function, but that doesn't end up with the result I am looking for. > This is fragile since you need to keep your unsupported list here in sync with the schema. Also at the moment this lands, we won't fail if `remove()` is called with something that we intend to support but don't currently support (eg, cache, history, etc) > How about iterating over the keys of dataToRemove instead, building your promises as necessary and rejecting if you encounter something unsupported. This is partially addressed by removing this check, which is no longer needed for permanently unsupported types because I am also removing them from the schema. One problem with this, though, is that we seem to no longer get a rejected promise if a caller tries to use an unsupported type, instead an error is thrown. Do you think that's ok? We do still need to address the issue of types that have not yet been implemented, and I like your idea, but how would I know that something is unsupported without maintaining a list somewhere of either what is or is not supported? Are you suggesting we could keep a list of what is supported and then add to it each time we ass support for another type? > why don't you add the creationTime as a parameter to `addCookie()` and back-date the cookies you're adding rather than waiting idly? The `add` method of the cookie service [1] does not support specifying a `creationTime`, and the docs for the cookie object itself [2] state that the `creationTime` is read only, so I assumed that was not possible. I can dig a bit more to see if it is possible to do this. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager2#add() [2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookie2
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review97246 > The `add` method of the cookie service [1] does not support specifying a `creationTime`, and the docs for the cookie object itself [2] state that the `creationTime` is read only, so I assumed that was not possible. I can dig a bit more to see if it is possible to do this. > > [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager2#add() > [2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookie2 After a bit of searching, I did come across an `insertCookie` method in head_cookies.js [1], but it relies on manually inserting a cookie into the cookies.sqlite file, which I am not keen on doing. I think we'd need to copy a bunch of code from head_cookies.js to do this and it sounds scary. I'll ask around on IRC to see if anyone has any opinions on trying to add a cookie with an explicit creationTime. [1] http://searchfox.org/mozilla-central/source/extensions/cookie/test/unit/head_cookies.js#398
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review97246 > After a bit of searching, I did come across an `insertCookie` method in head_cookies.js [1], but it relies on manually inserting a cookie into the cookies.sqlite file, which I am not keen on doing. I think we'd need to copy a bunch of code from head_cookies.js to do this and it sounds scary. I'll ask around on IRC to see if anyone has any opinions on trying to add a cookie with an explicit creationTime. > > [1] http://searchfox.org/mozilla-central/source/extensions/cookie/test/unit/head_cookies.js#398 I spoke to Marco in IRC and he confirmed that adding an API which allows one to set the creationTime on a cookie is highly unlikely. He suggested to write a method in head.js to update creationTime of a cookie directly on the db, so I guess I can go that route. I think that sounds simpler than it is, which it's probably worth doing.
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies There may be an issue with the code I am using for clearing cookies. I am simply using the function to clear cookies from sanitize.js [1], but looking at the code I can see that it not only clears cookies, but it also appears to clear plugin data [2]. In the code that is referred to as "clearing plugin cookies", but I notice it is pretty much the same code that is used to clear plugin data in ForgetAboutSite.jsm [3], so I am not clear on whether it actually has to do with cookies, or plugin data, or both. I am going to follow up with someone to see if I can better understand what this code is actually clearing, and if it is indeed plugin data (which is a separate datatype in browsingData), then I'll need to do something so that we only clear cookies, and not plugin data, when a user specifies "cookies" with browsingData. [1] http://searchfox.org/mozilla-central/source/browser/base/content/sanitize.js#233 [2] http://searchfox.org/mozilla-central/source/browser/base/content/sanitize.js#233 [3] http://searchfox.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm#88
Comment on attachment 8818093 [details] Bug 1321303 - Part 2: Implement browsingData.removeCache, As per [1], does not have the facility for timespan-based eviction, so we just have to clear everything. Do you think we need to do anything about this in our code, in terms of warning a user if they provide a since date? The issue is that they may be asking to clear a number of dataTypes and some may support since and others may not, so I'm not sure if we should issue a warning or not. For now I am not issuing a warning and simply clearing all the cache regardless of the since value. [1] http://searchfox.org/mozilla-central/source/browser/base/content/sanitize.js#207
(In reply to Bob Silverberg [:bsilverberg] from comment #15) > Comment on attachment 8818093 [details] > Bug 1321303 - Part 2: Implement browsingData.removeCache, > > As per [1], does not have the facility for timespan-based eviction, so we > just have to clear everything. Do you think we need to do anything about > this in our code, in terms of warning a user if they provide a since date? > The issue is that they may be asking to clear a number of dataTypes and some > may support since and others may not, so I'm not sure if we should issue a > warning or not. For now I am not issuing a warning and simply clearing all > the cache regardless of the since value. > > [1] > http://searchfox.org/mozilla-central/source/browser/base/content/sanitize. > js#207 Ignore this comment. Chrome's docs state that "when removing data, this clears the entire cache: it is not limited to the range you specify." [1] , so this is a non-issue. [1] https://developer.chrome.com/extensions/browsingData#type-DataTypeSet
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review98648 A few nitpicks but please get additional review of the body of `clearCookies()` from somebody more acquainted with that code. ::: browser/components/extensions/ext-browsingData.js:35 (Diff revision 7) > + // TODO: I'm not sure that we can just call sanitizer.items.cookies.clear, > + // so instead I am copying the code to clear cookies from sanitizer.js. > + // I am still in the process of trying to verify this though. > + // return sanitizer.items.cookies.clear(makeRange(options)); Can you get an additional review of this code from somebody more familiar with that code? ::: browser/components/extensions/ext-browsingData.js:60 (Diff revision 7) > + } > + } > + } else { > + // Remove everything. > + cookieMgr.removeAll(); > + yield new Promise(resolve => setTimeout(resolve, 0)); // Don't block the main thread too long. This doesn't appear necessary, you're about to return anyway ::: browser/components/extensions/ext-browsingData.js:73 (Diff revision 7) > + {message: "Firefox does not support protectedWeb or extension as originTypes."}); > + } > + > + let removalPromises = []; > + let invalidDataTypes = []; > + for (let dataType of Object.keys(dataToRemove)) { This can just be `for (let dataType in dataToRemove)` ::: browser/components/extensions/ext-browsingData.js:84 (Diff revision 7) > + default: > + invalidDataTypes.push(dataType); > + } > + } > + } > + if (invalidDataTypes.length) { The way this is constructed, if I call `remove({cookies: true, some_unimplemented_type: true})`, cookies will get cleared but the function will also fail. Is that the desired behavior? ::: browser/components/extensions/test/xpcshell/test_ext_browsingData.js:22 (Diff revision 7) > + try { > + browser.browsingData.remove({}, dataTypes); > + browser.test.assertTrue(false, `Expected error received when using ${dataType} dataType.`); > + } catch (e) { > + browser.test.assertTrue(/Type error for parameter dataToRemove/.test(e), > + `Expected error received when using ${dataType} dataType.`); > + } you can use browser.test.assertThrows here ::: browser/components/extensions/test/xpcshell/test_ext_browsingData.js:35 (Diff revision 7) > + > + // Test an unimplemented dataType. > + await browser.test.assertRejects( > + browser.browsingData.remove({}, {localStorage: true}), > + "Firefox does not support dataTypes: localStorage.", > + "Expected error received when using protectedWeb originType."); This needs to be updated ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_cookies.js:15 (Diff revision 7) > + name: "test_cookie", > + path: "/", > +}; > + > +function addCookie(cookie) { > + Services.cookies.add(cookie.host, cookie.path, cookie.name, "test", false, false, false, Date.now() / 1000 + 10); The last argument is expiration time right? No reason not to make it gigantic, 10 seconds sounds like we'll be flirting with intermittentns. ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_cookies.js:39 (Diff revision 7) > + permissions: ["browsingData"], > + }, > + }); > + > + async function testRemovalMethod(method) { > + addCookie(COOKIE); what does this do if the cookie already exists? in particular, the "old" cookie from the first invocation of `testRemovalMethod()` is still around the second time you enter `testRemovalMethod()`.
Attachment #8816194 - Flags: review?(aswan) → review+
Comment on attachment 8818093 [details] Bug 1321303 - Part 2: Implement browsingData.removeCache, https://reviewboard.mozilla.org/r/98182/#review98656 ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_cache.js:5 (Diff revision 4) > +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim: set sts=2 sw=2 et tw=80: */ > +"use strict"; > + > +add_task(async function testCache() { Separating all these tests is handy but something that's now missing is a test of `remove()` that clears more than one data type in a single call.
Attachment #8818093 - Flags: review?(aswan) → review+
Comment on attachment 8818313 [details] Bug 1321303 - Part 3: Implement browsingData.removeHistory, https://reviewboard.mozilla.org/r/98424/#review98660 ::: browser/components/extensions/ext-browsingData.js:34 (Diff revision 3) > let sanitizer = new Sanitizer(); > sanitizer.prefDomain = PREF_DOMAIN; > return sanitizer; > }); > > +function makeRange(options) { are you going to use this somewhere else? if not, can you just inline this code into clearHistory()?
Attachment #8818313 - Flags: review?(aswan) → review+
Comment on attachment 8818093 [details] Bug 1321303 - Part 2: Implement browsingData.removeCache, https://reviewboard.mozilla.org/r/98182/#review98656 > Separating all these tests is handy but something that's now missing is a test of `remove()` that clears more than one data type in a single call. True. Do you therefore think it would be better to combine all of the tests into one file so that the setup and logic can be shared? Would simply combining two of the datatypes (e.g., cookies and cache) into one file, and having a test that clears two data types suffice? Or do you think we need to test a bunch of different combinations of data types?
Comment on attachment 8818313 [details] Bug 1321303 - Part 3: Implement browsingData.removeHistory, https://reviewboard.mozilla.org/r/98424/#review98660 > are you going to use this somewhere else? if not, can you just inline this code into clearHistory()? Yes, I use it in downloads, which I've started working on. It was originally in clearCookies, and then I extracted it out of that when I needed it for both clearCookies and clearHistory, but then I had to rewrite clearCookies when I relaized it was doing more than just clearing cookies. The bottom line is yes, I will use it multiple times.
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review98648 > Can you get an additional review of this code from somebody more familiar with that code? I'm not sure who is responsible for cookies, but I'll ask Kris to take a look at it. > The way this is constructed, if I call `remove({cookies: true, some_unimplemented_type: true})`, cookies will get cleared but the function will also fail. Is that the desired behavior? Won't the `return` at line 85 cause none of the removals to happen? They happen when I call `Promise.all` at line 89, but how can that be called if I've already returned at line 85? > you can use browser.test.assertThrows here I tried to get `browser.test.assertThrows` to work, but all I get is an exception in the test. I.e., the exception I am expecting causes the test to fail and the test in fact hangs at that point. I tried to debug assertThrows, but I cannot even find it, and I don't see any of our tests that uses it. Where is the code for assertThrows? > what does this do if the cookie already exists? in particular, the "old" cookie from the first invocation of `testRemovalMethod()` is still around the second time you enter `testRemovalMethod()`. It overwrites it. I don't think it will cause any issues for the test, but I reversed the cases so I remove all cookies as the second step, and then there shouldn't be any cookies left around on the next iteration.
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies Kris, could you please review the code I added for `clearCookies` in ext-browsingData.js?
Attachment #8816194 - Flags: review?(kmaglione+bmo)
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review98648 > I'm not sure who is responsible for cookies, but I'll ask Kris to take a look at it. It looks like mak wrote most of the relevent sanitizer code... > Won't the `return` at line 85 cause none of the removals to happen? They happen when I call `Promise.all` at line 89, but how can that be called if I've already returned at line 85? No, as soon as you call `clearCookies()` the work begins. You're not ever waiting for that promise in the error case but the code still runs. `Promise.all()` just gives you a way to wait once for a list of existing promises to all resolve (or settle or whatever...) > I tried to get `browser.test.assertThrows` to work, but all I get is an exception in the test. I.e., the exception I am expecting causes the test to fail and the test in fact hangs at that point. I tried to debug assertThrows, but I cannot even find it, and I don't see any of our tests that uses it. Where is the code for assertThrows? How are you using it? You need to give it a callable so that it can wrap the invocation with a try, it sounds like you're not doing that... In any case, its implemented here: http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-c-test.js#154
Comment on attachment 8818093 [details] Bug 1321303 - Part 2: Implement browsingData.removeCache, https://reviewboard.mozilla.org/r/98182/#review98656 > True. Do you therefore think it would be better to combine all of the tests into one file so that the setup and logic can be shared? Would simply combining two of the datatypes (e.g., cookies and cache) into one file, and having a test that clears two data types suffice? Or do you think we need to test a bunch of different combinations of data types? I don't think you need to test every possible permutation of the types, its up to you how much you want to combine...
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies Marco, could you please review the code I added for `clearCookies` in ext-browsingData.js? It's pretty much copied from sanitize.js.
Attachment #8816194 - Flags: review?(kmaglione+bmo) → review?(mak77)
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review98648 > It looks like mak wrote most of the relevent sanitizer code... Ok, I have switched the review request to Marco, thanks. > No, as soon as you call `clearCookies()` the work begins. You're not ever waiting for that promise in the error case but the code still runs. `Promise.all()` just gives you a way to wait once for a list of existing promises to all resolve (or settle or whatever...) Ok, thanks for explaining that. I agree it's not good to clear some data and also reject the promise. To address this I have changed this from a rejection to a warning, which I think should be sufficient. Let me know if you think that's okay, or if we should still be throwing an exception, in which case I guess I need to reject the promise before I create any of the other promises.
Comment on attachment 8818093 [details] Bug 1321303 - Part 2: Implement browsingData.removeCache, https://reviewboard.mozilla.org/r/98182/#review98656 > I don't think you need to test every possible permutation of the types, its up to you how much you want to combine... Ok, for now I've combined the test files for cache and cookies, and I included tests for both cache and cookies individually, as well as a test that clears both of them. I think this is sufficient to prove that the logic in place for clearing more than one item at a time works.
Comment on attachment 8818623 [details] Bug 1321303 - Part 4: Implement browsingData.removeDownloads, https://reviewboard.mozilla.org/r/98634/#review99058 ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_downloads.js:36 (Diff revision 1) > + await publicList.add(download); > + > + // Confirm everything worked. > + let downloads = await publicList.getAll(); > + equal(downloads.length, 2, "2 Pretend downloads added."); > + ok((await downloadExists(publicList, OLD_NAME)), "Pretend old download exists."); nit: the message here confused me (i read "pretend" as a verb not an adjective) ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_downloads.js:70 (Diff revision 1) > + // Clear downloads with no since value. > + await setupDownloads(); > + extension.sendMessage(method, {}); > + await extension.awaitMessage("downloadsRemoved"); > + > + let publicList = await Downloads.getList(Downloads.PUBLIC); shouldn't this also be testing removal from the list of private browsing downloads?
Attachment #8818623 - Flags: review?(aswan) → review+
Depends on: 1324760
Comment on attachment 8819392 [details] Bug 1321303 - Part 5: Implement browsingData.removeFormData, https://reviewboard.mozilla.org/r/99182/#review101888 ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_formdata.js:15 (Diff revision 4) > +const REFERENCE_DATE = Date.now(); > + > +function checkZero(num, message) { equal(num, 0, message); } > +function checkOne(num, message) { equal(num, 1, message); } > + > +async function countEntries(fieldname, message, check) { This doesn't need to be async ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_formdata.js:19 (Diff revision 4) > + handleResult: function(result) { count = result; }, > + handleError: error => reject(error), > + handleCompletion: reason => { > + if (!reason) { > + check(count, message); > + resolve(); > + } > + }, This seems unnecessarliy messy, though perhaps its just the constraints of how FormHistory.jsm works. Does handleCompletion ever get called with a null or 0 value for reason? If so, what's the difference between that and handleError? Moreover, does handleCompletion ever get called more than once? If not, why not just put the check/resolve directly into handleResult? Separate from all that, you have a mix of styles here, why not just use `handleResult(result) { ... }` etc consistently? ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_formdata.js:23 (Diff revision 4) > + let callback = { > + handleResult: function(result) { count = result; }, > + handleError: error => reject(error), > + handleCompletion: reason => { > + if (!reason) { > + check(count, message); this is a nit but you only ever use countZero and countOne here, why not pass in the expected count and call `equal()` here directly instead of passing those callables? ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_formdata.js:34 (Diff revision 4) > + FormHistory.count({fieldname}, callback); > + }); > +} > + > +async function setupFormHistory() { > + async function searchEntries(terms, params) { not async ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_formdata.js:47 (Diff revision 4) > + > + FormHistory.search(terms, params, callback); > + }); > + } > + > + async function update(changes) { not async ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_formdata.js:50 (Diff revision 4) > + } > + > + async function update(changes) { > + return new Promise((resolve, reject) => { > + let callback = { > + handleError: error => reject(error), `handleError: reject` similarly for handleCompletion below
Attachment #8819392 - Flags: review?(aswan) → review+
Comment on attachment 8820741 [details] Bug 1321303 - Part 6: Implement browsingData.removePluginData, https://reviewboard.mozilla.org/r/100186/#review101890 r=me for the webextensions bits but the bulk of this is the test, it would be better reviewed by somebody familiar with the existing plugin code that the test uses. ::: browser/components/extensions/ext-browsingData.js:15 (Diff revision 2) > +XPCOMUtils.defineLazyModuleGetter(this, "TelemetryStopwatch", > + "resource://gre/modules/TelemetryStopwatch.jsm"); what is this for?
Attachment #8820741 - Flags: review?(aswan)
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords https://reviewboard.mozilla.org/r/100188/#review101894 ::: browser/components/extensions/ext-browsingData.js:98 (Diff revision 2) > + let logins = loginManager.getAllLogins(); > + for (let login of logins) { > + login.QueryInterface(Ci.nsILoginMetaInfo); > + if (login.timePasswordChanged > options.since) { > + loginManager.removeLogin(login); > + if (++yieldCounter % YIELD_PERIOD == 0) { Yuck. If we're going to do this, this test should be outside the enclosing if statement -- if somebody has many saved logins but gives a very recent `since` time, this can still iterate over all those entries without yielding. The choice of 10 seems awfully low though. ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_passwords.js:18 (Diff revision 2) > +const LOGIN_PASSWORD_FIELD = "password_field"; > +const OLD_HOST = "http://mozilla.org"; > +const NEW_HOST = "http://mozilla.com"; > + > +function addLogin(host, timestamp) { > + checkLoginExists(host, false); doesn't eslint complain about this coming before the definition? ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_passwords.js:41 (Diff revision 2) > + loginManager.removeAllLogins(); > + addLogin(NEW_HOST, REFERENCE_DATE); > + addLogin(OLD_HOST, REFERENCE_DATE - 10000); > +} > + > +add_task(async function testHistory() { update the function name
Attachment #8820742 - Flags: review?(aswan) → review+
Comment on attachment 8819392 [details] Bug 1321303 - Part 5: Implement browsingData.removeFormData, https://reviewboard.mozilla.org/r/99182/#review101888 > This seems unnecessarliy messy, though perhaps its just the constraints of how FormHistory.jsm works. Does handleCompletion ever get called with a null or 0 value for reason? If so, what's the difference between that and handleError? Moreover, does handleCompletion ever get called more than once? If not, why not just put the check/resolve directly into handleResult? > > Separate from all that, you have a mix of styles here, why not just use `handleResult(result) { ... }` etc consistently? Thanks for the suggestions. I took another look and was able to clean this up quite a bit.
Comment on attachment 8820741 [details] Bug 1321303 - Part 6: Implement browsingData.removePluginData, https://reviewboard.mozilla.org/r/100186/#review101890 > what is this for? Oops, leftover from when I was implementing the plugin clearing code myself. No longer needed.
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords https://reviewboard.mozilla.org/r/100188/#review101894 > Yuck. If we're going to do this, this test should be outside the enclosing if statement -- if somebody has many saved logins but gives a very recent `since` time, this can still iterate over all those entries without yielding. > The choice of 10 seems awfully low though. Maybe we don't need to do it at all? I just added it because something similar is done in `sanitize.js` when clearing cookies (which I've also included in our code that clears cookies). It seemed like a good idea, but maybe clearing passwords is fast enough to not have to worry about doing this. The number 10 was chosen as that's also what's being used in `sanitize.js` for cookies. As for where we yield, looking at the code for cookies, I infer that the issue isn't the iteration, but rather the time it takes to remove cookies. I assume that's why we only yield if we have removed a certain number of cookies (or passwords, in this case). The process will not be slow to iterate over a large number of items, if we are only removing a few - the issue is more about how long it will take to *remove* a large number of items. Having said all that, maybe removing cookies is slow, but removing passwords is not, so maybe we don't need this code here. I will try to find someone to ask about that.
Marco, I am requesting review from you for a couple of these patches. One is code that clears cookies, which is mostly copied over from `sanitize.js`, but needs a review. The other is code for clearing plugin data, which aswan would like someone else to review, specifically the tests. I'm not sure who to ask for the plugin data clearing stuff, but it looks like you also worked on that code in sanitize.js. If you are not the appropriate person to review that, do you know who is? Thanks. :)
Flags: needinfo?(mak77)
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords Hi Matt, Gijs suggested I ask you to take a look at the code I've written for removing logins in a WebExtensions API. I want to make sure I'm doing the right thing in the case that *a lot* of logins are removed by this method. I've implemented something similar to what we're doing for removing cookies (which is taken from sanitize.js). Would you mind taking a look and letting me know if this seems reasonable or if I need to do something else, or nothing at all?
Attachment #8820742 - Flags: feedback?(MattN+bmo)
Comment on attachment 8823441 [details] Bug 1321303 - Part 8: Implement browsingData serviceWorkers dataType, https://reviewboard.mozilla.org/r/101960/#review102858
Attachment #8823441 - Flags: review?(amarchesini) → review+
Blocks: 1328918
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords https://reviewboard.mozilla.org/r/100188/#review101894 > Maybe we don't need to do it at all? I just added it because something similar is done in `sanitize.js` when clearing cookies (which I've also included in our code that clears cookies). It seemed like a good idea, but maybe clearing passwords is fast enough to not have to worry about doing this. The number 10 was chosen as that's also what's being used in `sanitize.js` for cookies. As for where we yield, looking at the code for cookies, I infer that the issue isn't the iteration, but rather the time it takes to remove cookies. I assume that's why we only yield if we have removed a certain number of cookies (or passwords, in this case). The process will not be slow to iterate over a large number of items, if we are only removing a few - the issue is more about how long it will take to *remove* a large number of items. > > Having said all that, maybe removing cookies is slow, but removing passwords is not, so maybe we don't need this code here. I will try to find someone to ask about that. I discussed this with Gijs on IRC and he suggested I ask MattN for some advice, so I have done that in the bug. It sounds like we do need to keep something like this in place though.
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords Sorry, Matt, it looks like when I pushed a change to this bug to Mozreview it cancelled my feedback request that I made, but I am still looking for feedback. I have changes it to an r? instead. I am putting my question in here again as the feedback request seems to have disappeared. Sorry for the noise and thanks in advance for your help. ---------------------- Hi Matt, Gijs suggested I ask you to take a look at the code I've written for removing logins in a WebExtensions API. I want to make sure I'm doing the right thing in the case that *a lot* of logins are removed by this method. I've implemented something similar to what we're doing for removing cookies (which is taken from sanitize.js). Would you mind taking a look and letting me know if this seems reasonable or if I need to do something else, or nothing at all?
Attachment #8820742 - Flags: review?(MattN+bmo)
Attachment #8820741 - Flags: review?(aswan)
Attachment #8823441 - Flags: review?(aswan)
Comment on attachment 8816194 [details] Bug 1321303 - Part 1: Implement browsingData.remove and removeCookies https://reviewboard.mozilla.org/r/96958/#review104190
Attachment #8816194 - Flags: review?(mak77) → review+
Comment on attachment 8820741 [details] Bug 1321303 - Part 6: Implement browsingData.removePluginData, https://reviewboard.mozilla.org/r/100186/#review104200
Attachment #8820741 - Flags: review?(mak77) → review+
Comment on attachment 8820741 [details] Bug 1321303 - Part 6: Implement browsingData.removePluginData, https://reviewboard.mozilla.org/r/100186/#review104206 ::: browser/components/extensions/test/browser/browser_ext_browsingData_pluginData.js:14 (Diff revision 8) > +// Returns the chrome side nsIPluginTag for the test plugin. > +function getTestPlugin() { > + let tags = pluginHost.getPluginTags(); > + > + // Find the test plugin > + for (let i = 0; i < tags.length; i++) { for...of ::: browser/components/extensions/test/browser/browser_ext_browsingData_pluginData.js:16 (Diff revision 8) > + let tags = pluginHost.getPluginTags(); > + > + // Find the test plugin > + for (let i = 0; i < tags.length; i++) { > + if (tags[i].name == "Test Plug-in") { > + return tags[i]; or maybe just use array.find()? ::: browser/components/extensions/test/browser/browser_ext_browsingData_pluginData.js:53 (Diff revision 8) > + > + if (!something) { > + return false; > + } > + > + for (let i = 0; i < needles.length; ++i) { another possible array function, likely array.some()
I don't see anything blocking.
Flags: needinfo?(mak77)
(In reply to Bob Silverberg [:bsilverberg] from comment #49) > Created attachment 8820742 [details] > Bug 1321303 - Part 7: Implement browsingData.removePasswords Sorry Bob, I won't have time to get to this this week still. I have to finish some stuff up on Aurora before the merge around the 24th. You can redirect to dolske and/or split passwords to its own bug if you would like.
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #127) > (In reply to Bob Silverberg [:bsilverberg] from comment #49) > > Created attachment 8820742 [details] > > Bug 1321303 - Part 7: Implement browsingData.removePasswords > > Sorry Bob, I won't have time to get to this this week still. I have to > finish some stuff up on Aurora before the merge around the 24th. You can > redirect to dolske and/or split passwords to its own bug if you would like. Thanks Matt. I will redirect and if that doesn't work I'll split passwords off as you suggest.
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords Hi dolske, MattN suggested I redirect this r? to you. This is some code I've written for removing logins in a WebExtensions API. I want to make sure I'm doing the right thing in the case that *a lot* of logins are removed by this method. I've implemented something similar to what we're doing for removing cookies (which is taken from sanitize.js). Would you mind taking a look and letting me know if this seems reasonable or if I need to do something else, or nothing at all?
Attachment #8820742 - Flags: review?(MattN+bmo) → review?(dolske)
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords https://reviewboard.mozilla.org/r/100188/#review106892 ::: browser/components/extensions/ext-browsingData.js:21 (Diff revision 9) > XPCOMUtils.defineLazyServiceGetter(this, "cookieMgr", > "@mozilla.org/cookiemanager;1", > "nsICookieManager"); > +XPCOMUtils.defineLazyServiceGetter(this, "loginManager", > + "@mozilla.org/login-manager;1", > + "nsILoginManager"); You could just use Services.jsm here, for the nicer "Services.logins" and "Services.cookies". ::: browser/components/extensions/ext-browsingData.js:91 (Diff revision 9) > +let clearPasswords = Task.async(function* (options) { > + let yieldCounter = 0; > + > + if (options.since) { > + // Iterate through the logins and delete any updated after our cutoff. > + let logins = loginManager.getAllLogins(); Using .getAllLogins() is _mostly_ fine (read: good enough to land with), but you'll have a wee problem with Master Password. If the user has a MP set, but hasn't entered it yet, an extension calling this code will trigger a master password prompt, which the user probably isn't expecting. (And it's a little weird to have to enter your MP just to delete data.) If they enter it things will work fine, but if they don't getAllLogins() will throw. To fix this, we'll need to add support for removing logins by date range to nsILoginManager. Fodder for a separate bug, but it would seem simplest to just add an options argument to removeAllLogins with a "since" property. ::: browser/components/extensions/ext-browsingData.js:94 (Diff revision 9) > + if (options.since) { > + // Iterate through the logins and delete any updated after our cutoff. > + let logins = loginManager.getAllLogins(); > + for (let login of logins) { > + login.QueryInterface(Ci.nsILoginMetaInfo); > + if (login.timePasswordChanged > options.since) { The spec in comment 0 says .since means "Remove data accumulated on or after this date". So (nitpick) this should probably be >= instead of >.
Attachment #8820742 - Flags: review?(dolske) → review+
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords https://reviewboard.mozilla.org/r/100188/#review101894 > I discussed this with Gijs on IRC and he suggested I ask MattN for some advice, so I have done that in the bug. It sounds like we do need to keep something like this in place though. This has now been reviewed and r+'d by dolske.
Comment on attachment 8820742 [details] Bug 1321303 - Part 7: Implement browsingData.removePasswords https://reviewboard.mozilla.org/r/100188/#review106892 > Using .getAllLogins() is _mostly_ fine (read: good enough to land with), but you'll have a wee problem with Master Password. > > If the user has a MP set, but hasn't entered it yet, an extension calling this code will trigger a master password prompt, which the user probably isn't expecting. (And it's a little weird to have to enter your MP just to delete data.) If they enter it things will work fine, but if they don't getAllLogins() will throw. > > To fix this, we'll need to add support for removing logins by date range to nsILoginManager. Fodder for a separate bug, but it would seem simplest to just add an options argument to removeAllLogins with a "since" property. Thanks for the review and info, dolske. I have opened a couple of follow-up bugs to address this.
This looks good on try. Requesting check-in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0d944258ba3f Part 1: Implement browsingData.remove and removeCookies, r=aswan,mak https://hg.mozilla.org/integration/autoland/rev/34579b580023 Part 2: Implement browsingData.removeCache, r=aswan https://hg.mozilla.org/integration/autoland/rev/bcc9cdc7579c Part 3: Implement browsingData.removeHistory, r=aswan https://hg.mozilla.org/integration/autoland/rev/d8db138a2d4e Part 4: Implement browsingData.removeDownloads, r=aswan https://hg.mozilla.org/integration/autoland/rev/4b8193708793 Part 5: Implement browsingData.removeFormData, r=aswan https://hg.mozilla.org/integration/autoland/rev/95426f1151bc Part 6: Implement browsingData.removePluginData, r=mak https://hg.mozilla.org/integration/autoland/rev/8a161e6bae18 Part 7: Implement browsingData.removePasswords, r=aswan,Dolske https://hg.mozilla.org/integration/autoland/rev/4054f1c5e04a Part 8: Implement browsingData serviceWorkers dataType, r=baku
Keywords: checkin-needed
I've written some docs for this, please let me know if they make sense: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData
Flags: needinfo?(bob.silverberg)
Thanks Will. The docs look good. I have a few suggestions/corrections: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/RemovalOptions - Compatibility notes - should be changed to say that Firefox does not support protectedWeb or extension as originTypes. The originType of unprotectedWeb *is* supported. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/DataTypeSet - The list of types on MDN is incomplete. There are a few types that we do not support but that Chrome does. If the docs are meant to be cross-browser, those types should be included. You can see all of the types that Chrome supports at https://developer.chrome.com/extensions/browsingData#type-DataTypeSet - Firefox does support the serviceWorkers type, so that should be added to the list and listed as a supported type. Note that there is no corresponding removeServiceWorkers method. Many of the pages mention the RemovalOptions object and explain that it can be used to control removing objects from a particular origin. I know that on the page for RemovalOptions itself it does mention that originTypes is not supported (which is partly correct, and I corrected above), but it I think it would be good to have a compatibility note on each page that references RemovalOptions to let people know that Firefox doesn’t really support it. Otherwise the documentation seems misleading.
Flags: needinfo?(bob.silverberg) → needinfo?(wbamberg)
Thanks for the review Bob. I'll make those updates. (In reply to Bob Silverberg [:bsilverberg] from comment #145) > - The list of types on MDN is incomplete. There are a few types that we do > not support but that Chrome does. If the docs are meant to be cross-browser, > those types should be included. I don't think we should document something just because it's supported by Chrome. I think: * If we think it's Chrome-only, and it will not be part of the WebExtensions standard, then we should not document it. * If we do intend for it to be part of the standard, then we should document it (whether or not Firefox supports it). Under that principle, should I include all the data types listed for Chrome, or are there any that should be excluded?
Flags: needinfo?(wbamberg) → needinfo?(bob.silverberg)
(In reply to Will Bamberg [:wbamberg] from comment #146) > Thanks for the review Bob. I'll make those updates. > > (In reply to Bob Silverberg [:bsilverberg] from comment #145) > > > - The list of types on MDN is incomplete. There are a few types that we do > > not support but that Chrome does. If the docs are meant to be cross-browser, > > those types should be included. > > I don't think we should document something just because it's supported by > Chrome. I think: > > * If we think it's Chrome-only, and it will not be part of the WebExtensions > standard, then we should not document it. > > * If we do intend for it to be part of the standard, then we should document > it (whether or not Firefox supports it). > Those sounds like reasonable ideas. > Under that principle, should I include all the data types listed for Chrome, > or are there any that should be excluded? You should exclude appcache and webSQL. Neither of those will ever be supported in Firefox. The others might be in the future.
Flags: needinfo?(bob.silverberg) → needinfo?(wbamberg)
I think I've made all the changes you suggested in comment 145. Please let me know if it looks good now.
Flags: needinfo?(wbamberg) → needinfo?(bob.silverberg)
This looks good, Will. The only thing I'm wondering is if there should be a compatibility note on https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/DataTypeSet to discuss which types are not valid for Firefox.
Flags: needinfo?(bob.silverberg)
Thanks Bob. I didn't do this because DataTypeSet itself doesn't really do anything, so it can't support anything. It's remove() that doesn't support these options, so I noted it there instead.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: