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)

Product:
WebExtensions
Component:
Compatibility
Type:
defect

Tracking

(firefox53 fixed, firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [browsingData]triaged)

Attachments

(2 files)

Bug 1339868 - browsingData.settings includes an invalid property in its returned objects,
59 bytes, text/x-review-board-request
aswan
: review+
Details
browsingData.uplift.patch
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.
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 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+
Try looks good, requesting check in.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/a59d9440cef9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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 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+
(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
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: