812114 - Add an observer for the stop() method in private-browsing.js shared module

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andreea Matei [:AndreeaMatei]]

We need to add an observer for the "last-pb-context-exited" event in order to ensure the private browsing mode was exited.
This will be used in the new test for private browsing cache, in bug 789987.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Adding the observer to ensure the private browsing mode is exited.
Renamed observerService to obs as requested in bug 789987.

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

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

::: lib/private-browsing.js
@@ +202,5 @@
>        this._controller.mainMenu.click("#privateBrowsingItem");
>      }
>  
>      this.waitForTransistionComplete(false);
> +    assert.waitFor(function () {

I don't think we need assert.waitFor here, we could just use utils.waitFor.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated the assert.waitFor with utils.waitFor

Comment 4

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

Comment on attachment 682437 [details] [diff] [review]
patch v2

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

::: lib/private-browsing.js
@@ +203,5 @@
>  
>      this.waitForTransistionComplete(false);
> +    mozmill.utils.waitFor(function () {
> +      return finishedStateFlag;
> +    }, "Private browsing was exited");

Do we still need waitForTransitionComplete here? If not I wonder if we could use a similar observer topic for the start() method and get completely get rid of the above mentioned helper method.

If the observer doesn't get fired you have to make sure to remove it. This is mainly the reason for r-.

Comment 5

[Andreea Matei [:AndreeaMatei]]

I removed the waitForTransistionComplete and it seems it works fine without it as well. 
Regarding the observer for the start() method, I talked to Josh Matthews about the appropriate event, but there's none we can use.

I will get back with an update removing the waitForTransistionComplete call from stop().

Comment 6

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #5)
> Regarding the observer for the start() method, I talked to Josh Matthews
> about the appropriate event, but there's none we can use.

What a pity. But ok.

> I will get back with an update removing the waitForTransistionComplete call
> from stop().

Would it make sense to move the content from waitForTransistionComplete() in start() then? We do not need this method anymore.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Moved the waitForTransistionComplete() in start() method and removed it from stop().

Comment 8

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

Comment on attachment 685088 [details] [diff] [review]
patch v3

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

::: lib/private-browsing.js
@@ +210,5 @@
>      }
>  
> +    mozmill.utils.waitFor(function () {
> +      return finishedStateFlag;
> +    }, "Private browsing was exited");

As what I already have mentioned in comment 4 in case of a failure we miss to remove the registered observer and this will cause a leak.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4

Ensuring we remove the observer, even if we fail before, with a try-finally block.

Comment 10

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

Comment on attachment 686078 [details] [diff] [review]
patch v4

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

Looks good now. Only some nits to fix before we can land this.

::: lib/private-browsing.js
@@ +185,5 @@
>     * @param {boolean} useShortcut
>     *        Use the keyboard shortcut if true otherwise the menu entry is used
>     */
> +  stop: function privateBrowsing_stop(useShortcut) {
> +    var finishedStateFlag = false;

Can you move this down to the observer declaration?

@@ +191,5 @@
>      if (!this.enabled)
>        return;
>  
> +    // Set up an observer so we get notified when we exited PB
> +    let observer = {

Whether use var or let. I would say we stick with var as we do for all the other modules.

@@ +199,2 @@
>      }
> +    try {

Please keep a blank line above.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4.1

Updated the last requests.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 686555 [details] [diff] [review]
patch v4.1

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

Looks good. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/f5a6c6b9dfa5

Comment 13

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

I would like to get this patch backported. Do we know which branches support the observer?

Comment 14

[Andreea Matei [:AndreeaMatei]]

I've checked each branch, the failing ones are ESR10 (on 6 tests from private browsing) and ESR17 (on 1 test), with "Private Browsing was exited" error. I'll look into that more.
Aurora, Beta and Release work just fine and the patch applies cleanly to all.

Comment 15

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

ESR17 is based on the same code as release right now. So if we fail in ESR17 we should also fail on release. When has the observer been added actually?

Comment 16

[Andreea Matei [:AndreeaMatei]]

Oh, rechecked release now and indeed fails once, as ESR17, at testGeolocation.js.

But if the observer wouldn't be added there, we should have all of them failing.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm not following. What's failing? Could you paste a link to a dashboard report?

Comment 18

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

I know we failed once but I accidentally landed the new pb test on all other branches. If we still fail in CI we should raise a follow-up bug.

http://hg.mozilla.org/qa/mozmill-tests/rev/5e7226a6d415 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/92b9f7f67682 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/a1e39ce1c892 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/d011b6107dd4 (esr17)

Comment 19

[Andreea Matei [:AndreeaMatei]]

As expected, this is failing on ESR17, over platforms, for most of the private browsing tests:
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167aff9e2

Comment 20

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

Backout from esr17:
http://hg.mozilla.org/qa/mozmill-tests/rev/f25449c1ea43

Comment 21

[Andreea Matei [:AndreeaMatei]]

This happens on release as well:
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db0ffc63

The event's existence is not the issue since this is not reproducing constantly, we have passing testruns too.
When we fail, the flag is not getting changed to 'true', is 'false' all the way (and we do enter in all blocks - observer, try, waitFor, finally). Will keep investigating after we finish bug 818456 that is P1.

Comment 22

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

I would like to know what Josh or Ehsan think about it.

Comment 23

[Josh Matthews [:jdm]]

So is the issue that the test times out waiting here?

>   211       mozmill.utils.waitFor(function () {
>   212         return finishedStateFlag;
>  213       }, "Private browsing was exited");

Comment 24

[Andreea Matei [:AndreeaMatei]]

Yep, sometimes that value is not getting changed in the observer, where we set it true.

Comment 25

[Josh Matthews [:jdm]]

I don't have any particular suggestions, unfortunately. That suggests that the observer isn't triggered, which indicates that private browsing is still in effect :/

Comment 26

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

Andreea, can you watch such a testrun or slow it down so we might see what's going on? Are all private browsing windows getting closed?

Comment 27

[Andreea Matei [:AndreeaMatei]]

Investigated some more and this is not failing anymore on release. That is because the way "last-pb-context-exited" occurs has changed from release up, explained Josh.

Only ESR17 is affected, which can be fixed by using "private-browsing-transition-complete" instead of "last-pb-context-exited".
Uploading the patch and reports soon.

Comment 28

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #27)
> Only ESR17 is affected, which can be fixed by using
> "private-browsing-transition-complete" instead of "last-pb-context-exited".
> Uploading the patch and reports soon.

So the notification message has been changed then? That would totally explain it. Sounds we are on the right track. Thanks Andreea!

Comment 29

[Josh Matthews [:jdm]]

No, there was no change. The difference is in bug 804653, in which the notification becomes independent of non-deterministic factors like garbage collection.

Comment 30

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

(In reply to Josh Matthews [:jdm] from comment #29)
> No, there was no change. The difference is in bug 804653, in which the
> notification becomes independent of non-deterministic factors like garbage
> collection.

So could we use this test as a test for bug 804653? If yes, how should it handle the different kinds of notifications?

Comment 31

[Josh Matthews [:jdm]]

No. The previous behaviour was non-deterministic (eg. the test fails 1/20 times), which is not good for automated tests.

Comment 32

[Andreea Matei [:AndreeaMatei]]

Attachment: [ESR17] fix patch

Using the 'private-browsing-transition-complete' notification that fixes the issue.

Reports:
* OS X: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb5105d4af
* Linux: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb510599fa
* Windows: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51064e85

Comment 33

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

Comment on attachment 704791 [details] [diff] [review]
[ESR17] fix patch

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

Please try to keep the code as best as possible in sync with the other patch. This will save us a lot of trouble when backporting patches. I would have expected that you only replace the notification topic string.

Comment 34

[Andreea Matei [:AndreeaMatei]]

Attachment: [ESR17] patch

The checked-in patch does no longer apply to ESR17.
Left the observer service (it will be taking care of in bug 830688) and removed the constant for the notification, to be in sync.

Comment 35

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

Comment on attachment 704823 [details] [diff] [review]
[ESR17] patch

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

Looks good now but please add a comment for the observer topic used so that we can remember why we have such a difference compared to the other branches. Also include the bug you and Josh were referring to. Once uploaded you can carry forward the r+ flag.

Comment 36

[Andreea Matei [:AndreeaMatei]]

Attachment: [ESR17] fix patch

Added the comment for the notification.

Comment 37

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

Comment on attachment 704858 [details] [diff] [review]
[ESR17] fix patch

http://hg.mozilla.org/qa/mozmill-tests/rev/9b2e455887a2 (esr17)

Comment 38

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

Sweet, we are done!