If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Don't sync history removals caused by expiration

RESOLVED FIXED in mozilla7

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({dataloss})

unspecified
mozilla7
dataloss
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-places])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Since expiration can act differently based on the device (being much more active on mobile), Sync should not sync down expiration removes from mobile to desktop and viceversa.
Huh, we didn't do this before, IIRC, what changed?
(Assignee)

Comment 2

6 years ago
afaict Sync always did this, since there is no way to distinguish automatic removals from user removals. you just get notified visits or pages are removed, not why.
(Assignee)

Comment 3

6 years ago
Created attachment 541482 [details] [diff] [review]
patch v1.0

I suppose the test will be harder than the patch :p
(Assignee)

Comment 4

6 years ago
Created attachment 541483 [details] [diff] [review]
patch v1.1

but I was able to do it wrong!
Attachment #541482 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 541486 [details] [diff] [review]
patch v1.2

sigh :(
Attachment #541483 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 541569 [details] [diff] [review]
tests v1.0

waiting on the blocking bug before proceeding with review.
(Assignee)

Updated

6 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: in-testsuite?
(Assignee)

Updated

6 years ago
Attachment #541486 - Flags: review?(philipp)
(Assignee)

Updated

6 years ago
Attachment #541569 - Flags: review?(philipp)
Comment on attachment 541569 [details] [diff] [review]
tests v1.0

>+  // Observe expiration.
>+  Services.obs.addObserver(function (aSubject, aTopic, aData) {
>+    Services.obs.removeObserver(arguments.callee, aTopic);

arguments.callee is deprecated. I suggest giving the function a name and then referring to the name in the next line, e.g.:

  Services.obs.addObserver(function onExpirationFinished(aSubject, aTopic, aData) {
    Services.obs.removeObserver(onExpirationFinished, aTopic);

>+  Cc["@mozilla.org/places/expiration;1"].
>+  getService(Ci.nsIObserver).
>+  observe(null, "places-debug-start-expiration", 1);

Nit: please indent the two lines following the first. In Sync code we also follow the convention of putting the '.' operation at the start of the continuation line:

  Cc["@mozilla.org/places/expiration;1"]
    .getService(Ci.nsIObserver)
    .observe(null, "places-debug-start-expiration", 1);

(This is how it's documented and recommended in the Coding Style [1] for more robustness with JavaScript keywords. Although some claim that document is "wrong"... I wonder why we have it at all then...

[1] https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Operators)


r=me. You can land this on the Places branch if you want to.
Attachment #541569 - Flags: review?(philipp) → review+
Attachment #541486 - Flags: review?(philipp) → review+
(In reply to comment #7)
> r=me. You can land this on the Places branch if you want to.

I should point out that bug 665965 touches the neighbouring lines and will likely cause a merge conflict. It might make sense for you to wait until it has landed on m-c with the next s-c train on Tuesday, so you can rebase your patch on top of it. Just FYI :)
(Assignee)

Comment 9

6 years ago
(In reply to comment #7)
> Comment on attachment 541569 [details] [diff] [review] [review]
> tests v1.0
> 
> >+  // Observe expiration.
> >+  Services.obs.addObserver(function (aSubject, aTopic, aData) {
> >+    Services.obs.removeObserver(arguments.callee, aTopic);
> 
> arguments.callee is deprecated.

arguments.callee is not deprecated as used here:
"JavaScript 1.4: Deprecated callee as a property of Function.arguments, retained it as a property of a function's local arguments variable."
A lot of stuff would break globally if we'd ever deprecate it, most of the patches I've seen lately use it as well.

> Nit: please indent the two lines following the first. In Sync code we also
> follow the convention of putting the '.' operation at the start of the
> continuation line:

well actually it's not completely true, the code style followed till now is dot on next line, but not on xpcom services. like
let xyz = test.this
    .that
but
let xyz = CC[this].
          that

No idea why though.
(Assignee)

Comment 10

6 years ago
Ehr sorry, comment got cut.
I was saying I'll follow Sync code style as you prefer, no problem, but we do differently around.
(In reply to comment #9)
> arguments.callee is not deprecated as used here:
> "JavaScript 1.4: Deprecated callee as a property of Function.arguments,
> retained it as a property of a function's local arguments variable."

Looks like https://developer.mozilla.org/en/JavaScript/Reference/Functions_and_function_scope/arguments/callee needs to be updated quite badly, because arguments.callee is no longer available in ES5 strict mode:

  (function () {"use strict"; return arguments.callee;})()

will throw:

  TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them

> A lot of stuff would break globally if we'd ever deprecate it, most of the
> patches I've seen lately use it as well.

That's really unfortunate. I recommend reviewers call this out.

> > Nit: please indent the two lines following the first. In Sync code we also
> > follow the convention of putting the '.' operation at the start of the
> > continuation line:
> 
> well actually it's not completely true, the code style followed till now is
> dot on next line, but not on xpcom services.

Weird. Well, it would be nice if within Sync at least the style would be consistent :)
(Assignee)

Comment 12

6 years ago
(In reply to comment #11)
> (In reply to comment #9)
> > arguments.callee is not deprecated as used here:
> > "JavaScript 1.4: Deprecated callee as a property of Function.arguments,
> > retained it as a property of a function's local arguments variable."
> 
> Looks like
> https://developer.mozilla.org/en/JavaScript/Reference/
> Functions_and_function_scope/arguments/callee needs to be updated quite
> badly, because arguments.callee is no longer available in ES5 strict mode:

Good point, there should be more blog posts when these things change, will fix it and start acting accordingly.
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/projects/places/rev/13e50174a62f
Whiteboard: [fixed-in-places]
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/mozilla-central/rev/13e50174a62f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.