Closed
Bug 1339868
Opened 7 years ago
Closed 7 years ago
browsingData.settings includes an invalid property in its returned objects
Categories
(WebExtensions :: Compatibility, defect, P2)
WebExtensions
Compatibility
Tracking
(firefox53 fixed, firefox54 fixed)
RESOLVED
FIXED
mozilla54
People
(Reporter: bsilverberg, Assigned: bsilverberg)
References
Details
(Whiteboard: [browsingData]triaged)
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
7.65 KB,
patch
|
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Thanks to wbamberg for finding this. When the settings are returned by browsingData.settings, the dataToRemove object and the dataRemovalPermitted object both include both a `formData` property and a `formdata` property. The latter is invalid.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
This bug has been expanded to also include bug 1339881 which is: browsingData.settings() does not work if "Time range to clear" is set to "Everything". A patch will be added to address both issues.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8837733 [details] Bug 1339868 - browsingData.settings includes an invalid property in its returned objects, https://reviewboard.mozilla.org/r/112762/#review114720 ::: browser/components/extensions/ext-browsingData.js:189 (Diff revision 2) > // The following prefs are the only ones in Firefox that match corresponding > // values used by Chrome when rerturning settings. > const PREF_LIST = ["cache", "cookies", "history", "formdata", "downloads"]; > > // since will be the start of what is returned by Sanitizer.getClearRange > // divided by 1000 to convert to ms. add a comment about "Everything" here ::: browser/components/extensions/ext-browsingData.js:205 (Diff revision 2) > // formData has a different case than the pref formdata. > dataToRemove.formData = Preferences.get(`${PREF_DOMAIN}formdata`); > dataRemovalPermitted.formData = true; I think this would be more readable if you move it inside the loop and do something like: ``` const name = item == "formdata" ? "formData" : item; dataToRemove[name] = ...; dataRemovalPermitted[name] = ...; ``` ::: browser/components/extensions/test/xpcshell/test_ext_browsingData_settings.js:67 (Diff revision 2) > do_register_cleanup(() => { > - branch.clearUserPref(SINGLE_PREF); > + Preferences.reset(SINGLE_PREF); > + Preferences.reset("privacy.sanitize.timeSpan"); > }); > > - branch.setBoolPref(SINGLE_PREF, true); > + // Test with "since" set to "Everything", as that is an edge case. I may be misunderstanding the logic here but I'm finding the way different things are combined here a little confusing. I think it would be easier to follow if there was a set of tests that vary the timeSpan setting and check the valud of `since` that comes back, then a different set of tests that flip prefs and check `dataToRemove` properties.
Attachment #8837733 -
Flags: review?(aswan) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a59d9440cef9 browsingData.settings includes an invalid property in its returned objects, r=aswan
Keywords: checkin-needed
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a59d9440cef9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e70f72a6511
Assignee | ||
Comment 12•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: 1339868 / 1339881 [User impact if declined]: This patch addresses two bugs in the WebExtensions browsingData API, one of which will cause the API to throw an error if a user has a particular setting configured for clearing data. [Is this code covered by automated tests?]: Yes, new tests were added that cover all of the code changes. [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No. [Why is the change risky/not risky?]: This is a small change to a single file which only implements the browsingData API. The changes are fully covered by automated tests. A try run has been submitted at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e0ceb6f17066285aac91542ea1f012e7850508a [String changes made/needed]: None
Attachment #8839918 -
Flags: approval-mozilla-aurora?
Comment 13•7 years ago
|
||
Comment on attachment 8839918 [details] [diff] [review] browsingData.uplift.patch Fix an issue related to WebExtensions browsingData API. Aurora53+.
Attachment #8839918 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•7 years ago
|
status-firefox53:
--- → affected
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #12) > Created attachment 8839918 [details] [diff] [review] > browsingData.uplift.patch > > Approval Request Comment > [Feature/Bug causing the regression]: 1339868 / 1339881 > [User impact if declined]: This patch addresses two bugs in the > WebExtensions browsingData API, one of which will cause the API to throw an > error if a user has a particular setting configured for clearing data. > [Is this code covered by automated tests?]: Yes, new tests were added that > cover all of the code changes. > [Has the fix been verified in Nightly?]: No. > [Needs manual test from QE? If yes, steps to reproduce]: No > [List of other uplifts needed for the feature/fix]: None > [Is the change risky?]: No. > [Why is the change risky/not risky?]: This is a small change to a single > file which only implements the browsingData API. The changes are fully > covered by automated tests. A try run has been submitted at > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=7e0ceb6f17066285aac91542ea1f012e7850508a > [String changes made/needed]: None The bug number for the feature that introduced this was 1320986
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/afd377db9e37
Flags: in-testsuite+
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•