815605 - Ensure the registered observer gets removed in places.js shared module

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andreea Matei [:AndreeaMatei]]

At this moment, in removeAllHistory() method from places.js we don't ensure the removal of the observer, in case of a failure. We need to update that.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Ensuring we remove the observer with a try-finally block.

Comment 2

[Andreea Matei [:AndreeaMatei]]

With this changed I don't get any more "Browsing History has been cleared" failures, at least on default.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 689628 [details] [diff] [review]
patch v1

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

::: lib/places.js
@@ +104,5 @@
>  function removeAllHistory() {
>    const TOPIC_EXPIRATION_FINISHED = "places-expiration-finished";
>  
> +  // Set up an observer and a flag so we get notified when remove completes
> +  var finishedStateFlag = false;

Nit for the future. Shorter names are preferred. A simple 'finished' would have been enough.

@@ +107,5 @@
> +  // Set up an observer and a flag so we get notified when remove completes
> +  var finishedStateFlag = false;
> +  var observer = {
> +    observe: function (aSubject, aTopic, aData) {
> +      finishedStateFlag = true;

I still think we should also remove the observer here. Otherwise another notification could be caught which you would expect for another action. But this can be a follow-up bug. Please make sure to file it. And it should include all instances where we make use of observers in our libs and tests.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

http://hg.mozilla.org/qa/mozmill-tests/rev/77fb1ae5fe44 (default)

(In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> With this changed I don't get any more "Browsing History has been cleared"
> failures, at least on default.

Lets see if that is true. If yes we can even close bug 800800.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> With this changed I don't get any more "Browsing History has been cleared"
> failures, at least on default.

It's still happening on default:
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167f01644

So this change doesn't fix bug 800800.

Comment 6

[Andreea Matei [:AndreeaMatei]]

That is a different failure, the one in bug 800800 is "Browsing History has been cleared" from removeAllHistory(), reproducible on Windows only.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right, my fault. Please make sure to get it filed cause it seems to be regressed by a feature change in Firefox.

Comment 8

[Andreea Matei [:AndreeaMatei]]

This works as expected and the patch applies cleanly to all other branches.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you Andreea. Landed on all other branches:

http://hg.mozilla.org/qa/mozmill-tests/rev/7d9eb65fb583 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/7c884448617a (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/3f37a7148c49 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/1f93684a26e6 (esr17)
http://hg.mozilla.org/qa/mozmill-tests/rev/885f7932d9da (esr10)