1233628 - handling {} for clear-origin-data in PushService.jsm

Status: RESOLVED INVALID

(Core :: DOM: Push Subscriptions, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See Honza's comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1233136#c0

"PushService [1], bug 1170115, that seems to do the right thing, except that empty aData string is simply ignored (the code doesn't delete anything)"

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ac31c2d235f

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Comment 3

[Lina Butler (ex-Mozilla)]

Comment on attachment 8699852 [details] [diff] [review]
Patch

Review of attachment 8699852 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! r=me with the two issues fixed.

::: dom/push/PushService.jsm
@@ -295,1 @@
>      let pattern = JSON.parse(data);

This doesn't seem quite right. `JSON.parse("")` will throw a syntax error. How about `data ? JSON.parse(data) : {}`?

::: dom/push/test/xpcshell/test_clear_origin_data.js
@@ +8,5 @@
>  const userAgentID = 'bd744428-f125-436a-b6d0-dd0c9845837f';
>  
>  let clearForPattern = Task.async(function* (testRecords, pattern) {
>    let patternString = JSON.stringify(pattern);
> +  yield PushService.observe(null, 'clear-origin-data', patternString);

Please change `observe` to return the promise. Otherwise, this might race.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Oh your're right, I made a mistake, I thought JSON.stringify("{}") will become "" but it should be "{}", so the original code should work already, I'll go back to bug 1233136 to confirm with Honza again.

Thanks