885221 - controller.waitForPageLoad() failures with mozmill 2.0 due to closeAllTabs method opening about:newtab instead of about:blank

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect, P1)

Description

[Daniela Petrovici]

The changes in bug 764782 caused regressions when running mozmill-tests with mozmill 2.0.
These are reports from Linux and Mac where tests are failing with controller.waitForPageLoad error.
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cffdc3eb2
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cffdcde03

When changing back the closeAllTabs method to open about:blank instead of about:newtab, the issue does not reproduce anymore.

Comment 1

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

This is somewhat important we have to investigate and fix as soon as possible. Who can take this?

Comment 2

[Daniela Petrovici]

I will work on this issue

Comment 3

[Daniela Petrovici]

Attachment: logs for using about:newtab

I will attach the logs for using about:blank in a moment, too.

The problem is that the about:newtab is already loaded and waiting for it fails.
Removing the line below solves the issue in mozmill 2.0 and does not affect mozmill 1.5:
http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/tabs.js#l164

Wait for page load fails for about:newtab because the load_current and load_handled are the same and isLoaded will always be false - line: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/windows.js#L118
Also, based on the below two dumps, the about:newtab is already loaded

1) Using about:newtab
========================PAGE HIDE ===================================
*** 'DOMContentLoaded' event: id=8, baseURI=about:newtab
*** 'pageshow' event: id=8, baseURI=about:newtab
* current map: {"3":{"loaded":true},"8":{"loaded":true,"id_load_in_transition":{"name":"","number":"{5dc9dd4d-267d-4e17-8f29-834509c029af}","valid":true},"id_load_handled":{"name":"","number":"{5dc9dd4d-267d-4e17-8f29-834509c029af}","valid":true}}}

2) using about:blank
========================PAGE HIDE ===================================
*** 'pagehide' event: id=8, baseURI=about:blank
* current map: {"3":{"loaded":true},"8":{"loaded":false,"id_load_in_transition":{"name":"","number":"{b7713631-6a7a-4103-84bf-d4fc3cef2908}","valid":true},"id_load_handled":{"name":"","number":"{b7713631-6a7a-4103-84bf-d4fc3cef2908}","valid":true}}}
* current map: {"3":{"loaded":true},"8":{"loaded":false,"id_load_in_transition":{"name":"","number":"{36b40574-3e12-4ed1-b0d8-e33474cacdef}","valid":true},"id_load_handled":{"name":"","number":"{b7713631-6a7a-4103-84bf-d4fc3cef2908}","valid":true}}}

We are also having lines 260-264 in window.js that are never reached. I have added dumps inside both if and else and they were not displayed. I am not sure if this is important, but wanted to mention because, based on the comment before these lines, it seems that we are checking if the window has finished loading
(https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/windows.js#L260)

I think that waitForPageLoad does not check if the page (about:newtab in our case) has been preloaded or not.

Comment 4

[Daniela Petrovici]

Attachment: logs for using about:blank

Comment 5

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

(In reply to Daniela Petrovici from comment #3)
> Wait for page load fails for about:newtab because the load_current and
> load_handled are the same and isLoaded will always be false - line:
> https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/
> resource/modules/windows.js#L118

This only happens when a call to waitForPageLoad() has already happened for this specific content window. Only that method sets the load_handled attribute for the given window id in the window map. Means another waitForPageLoad() will fail, that's correct.

Also I do not see that you have taken the advice and checked with the pre-load preference disabled. How does this change the behavior?

> We are also having lines 260-264 in window.js that are never reached. I have
> added dumps inside both if and else and they were not displayed. I am not

Those lines will only be executed when a new ChromeWindow gets opened. Means you will only see those dumps at the very beginning of the test. Otherwise start Mozmill with --manual, and press Accel+N to open a new window. Then those messages should also be dumped to the terminal.

Comment 6

[Daniela Petrovici]

Attachment: logs using about:newtab

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Also I do not see that you have taken the advice and checked with the
> pre-load preference disabled. How does this change the behavior?
It doesn't change the behavior for mozmill 2.0, but it does for mozmill 1.5. So, adding that preference in mozmill 1.5 makes the testNewTab.js pass (bug 879752), but with mozmill 2.0, the tests are not passing when adding that preference.

If using about:newtab:
*testCloseTab*

1) wait for about:newtab to load in setupModule - tabs.closeAllTabs()

*** 'DOMContentLoaded' event: id=49, baseURI=about:newtab
*** 'pageshow' event: id=49, baseURI=about:newtab
* current map: {"3":{"loaded":true},"49":{"loaded":true,"id_load_in_transition":{"name":"","number":"{e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}","valid":true},"id_load_handled":{"name":"","number":"{563b1469-4a89-4e59-9607-92504f44dd84}","valid":true}}}
*** Page status updated: id=49, loaded=true, uuid={e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}

2) opens about:blank in testCloseTab through openTab()

So, about:newtab is loaded now, but about:blank is not:

*** 'beforeunload' event: id=54, baseURI=about:blank
* current map: {"3":{"loaded":true},"49":{"loaded":true,"id_load_in_transition":{"name":"","number":"{e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}","valid":true},"id_load_handled":{"name":"","number":"{e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}","valid":true}},"54":{"loaded":false}}
* current map: {"3":{"loaded":true},"49":{"loaded":true,"id_load_in_transition":{"name":"","number":"{e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}","valid":true},"id_load_handled":{"name":"","number":"{e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}","valid":true}},"54":{"loaded":false,"id_load_in_transition":{"name":"","number":"{2e186475-75e7-4ee0-ab28-b08381883114}","valid":true}}}
*** Page status updated: id=54, loaded=false, uuid={2e186475-75e7-4ee0-ab28-b08381883114}


testNewWindow
1) opens about:blank and waits for it to load, but the page appears to be loaded (although in the previous test it wasn't loaded)

*** 'DOMContentLoaded' event: id=49, baseURI=about:blank
*** 'pageshow' event: id=49, baseURI=about:blank
* current map: {"3":{"loaded":true},"49":{"loaded":true,"id_load_in_transition":{"name":"","number":"{e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}","valid":true},"id_load_handled":{"name":"","number":"{e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}","valid":true}}}
*** Page status updated: id=49, loaded=true, uuid={e62f42d7-c2e6-4266-8aa7-3e38edb66ed8}

So, based on the logs it seems that when using about:new tab, the page with ID=49 in testCloseTab is about:newtab. In testNewWindow, the page with ID=49 is about:blank and the UUID is the same in both tests.
I may be wrong about this, but I am not sure we are clearing the map correctly after one test.

Comment 7

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

(In reply to Daniela Petrovici from comment #6)
> It doesn't change the behavior for mozmill 2.0, but it does for mozmill 1.5.
> So, adding that preference in mozmill 1.5 makes the testNewTab.js pass (bug
> 879752), but with mozmill 2.0, the tests are not passing when adding that
> preference.

So I wouldn't do that change then. Lets try to get this fixed for real instead.

> I may be wrong about this, but I am not sure we are clearing the map
> correctly after one test.

We never clear the map entries. This all is done inside the window handling code in Mozmill. Each time a tab gets closed its internal content window will be removed. Same if a page gets reloaded. Then a new id will be added to the map and the old gets removed.

I might have to take a look at this issue.

Comment 8

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

Daniela, is this still a problem for you, even after my patch on bug 865641 landed?

Comment 9

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Daniela, is this still a problem for you, even after my patch on bug 865641
> landed?
The issue still reproduces with /testAwesomeBar/testAccessLocationBar.js. Please see reports:
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc94753757d
- without the change from bug 764782:
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc94753c782

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc9475387f4
- without the change from bug 764782:
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc94753cbd5

Since the issue started reproducing after bug 764782 was fixed, I think the change in bug 865641 fixed part of the issue, but not all of it.

Also, the test where this issue reproduces does not contain closeAllTabs method. The previously run test (http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testAddons/testManagerKeyboardShortcut.js) does. 

I will continue investigating this.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

I investigated and I found out that:
-about:blank triggers a pagehide event, before the new page triggers a pageshow event, this will change the uuid of page 
-about:newTab  does not trigger the pagehide event, so the new page will trigger a  pageshow event only,  but the uuid will be the same so waitForPageLoad  will fail

I changed the the preferences as you said on "Ask an Expert", I set the "browser.newtab.preload" to false:
- directly in the test 
- directly in mozmill 
- i put a sleep, and I changed it manually
And the bug still reproduces

Should I still file a new bug for mozmill 2.0?

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: simplifiedTC1.js

Created simplified test cases for reproducing the issue. It will fail in test 2 that I will attach in a moment

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: simplifiedTC2.js

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: simplifiedTC2.js

I investigated this and still didn't found out why after changing the tab navigating from newtab to newtab doesn't trigger an pagehide or beforeunload event, causing uuid of page to be the same so waitForPageLoad fails.

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

I removed this condition because pagehide event has attribute persisted set to false after we change the tab (see the minimizedTC) and beforeunload event is not triggered.
Whitout this condition, we relay on pagehide event to update the page status, and remove the beforeunload listener, and we falback to beforeunload event only if pagehide is not triggered.
Whit this change we have full green testrun on mozmill 2.0 

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/windows.js#L225

Reports
http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace67445118b50
http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace674451195e7
http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace6744511a582

Comment 16

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

Comment on attachment 799332 [details] [diff] [review]
patch_v1.0

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

Please add a mutt test, which can be created from your minimized testcases and check that it fails without your patch and passes with the patch included.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0 mutt test

Here is the patch with mutt tests, it fails without my patch applied.

Comment 18

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

Comment on attachment 800086 [details] [diff] [review]
patch_v1.0 mutt test

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

Please combine both patches into a single one. Other than that it looks fine given that it reproduces the problem.

::: mutt/mutt/tests/js/testUtils/manifest.ini
@@ +4,4 @@
>  [testPageLoad.js]
>  [testPageLoadBfCache.js]
>  [testSaveScreenshot.js]
> +[testPageLoadAfterTabChange.js]
\ No newline at end of file

There is no need for a new test file. Lets get it added to testPageLoad.js. Also the test method should contain something like 'ReloadPageAfterTabChange'

Comment 19

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

Comment on attachment 799332 [details] [diff] [review]
patch_v1.0

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

Please add a mutt test, which can be created from your minimized testcases and check that it fails without your patch and passes with the patch included.

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ -221,5 @@
>  
>    var pageHideHandler = function (aEvent) {
> -    // If event.persisted is false, the beforeUnloadHandler should fire
> -    // and there is no need for this event handler.
> -    if (aEvent.persisted) {

I'm a bit worries about that removal. Does it not regress something else? Can you please check when it has been introduced? It would be good to have a reference we can check why it is/was necessary.

If persisted is really set to false, the unload handler should be called right after pageHide as described here: https://developer.mozilla.org/en-US/docs/Using_Firefox_1.5_caching

So I'm not sure if this is the right fix for the problem. Would you mind to run this minimized test with all dump statements in that file and controller.js active? I would like to see the output.

Comment 20

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

Cosmin, I'm waiting for an update here. Please fix the mentioned issues and combine both patches into a single one ASAP. We want to get the next RC of Mozmill released very soon. Thanks.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

Sorry for taking so long, I combined the two patches, and renamed the test. 

(In reply to Henrik Skupin (:whimboo) from comment #19)
> Comment on attachment 799332 [details] [diff] [review]
> I'm a bit worries about that removal. Does it not regress something else?
> Can you please check when it has been introduced? It would be good to have a
> reference we can check why it is/was necessary.
It doesn't regresses  anything as far as I seen, because all mutt tests are green and also this makes the testrun to be green.

I return with logs in a moment.

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: logs.txt

Here are the dumps.

Comment 24

[Cosmin Malutan, [:cosmin-malutan]]

From what I understood, the beforeunload will fire if the page is not cached and I think it does not catch XUL pages.

Comment 25

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

Comment on attachment 804492 [details]
logs.txt

Thanks for the log, but it is not helpful to me to show me a passing testrun. I really want to see the failure which is happening here. So I will test it myself now locally.

Comment 26

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

Comment on attachment 804491 [details] [diff] [review]
patch_v2.0

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

As said earlier on that bug I'm not going to r+ this patch. Main reason I still think it's not the right solution and just a workaround for this particular case. I still feel we will open up a can of worms with it for other pages. A quick check showed me that all the events we get are getting here are correctly handled. Something in tracking the status seems to be wrong here. I hope you don't have any problems when I take it over?

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +230,4 @@
>      }
> +
> +    // If pagehide was fired we don't have to listen for beforeunload event anymore
> +    aWindow.removeEventListener("beforeunload", beforeUnloadHandler, true);

No, this will not always be the case. You can only assume that beforeunload is not being called if aEvent.persisted is true, means when the patch has been cached. You remove that logic here.

Also I don't see a pagehide event for about:newtab. It only occurs for the test before: http://localhost:43336/link.html

Comment 27

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

Comment on attachment 804491 [details] [diff] [review]
patch_v2.0

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

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +230,4 @@
>      }
> +
> +    // If pagehide was fired we don't have to listen for beforeunload event anymore
> +    aWindow.removeEventListener("beforeunload", beforeUnloadHandler, true);

Ok, so the given solution here is not that devastating as I first thought of. So we indeed have the pagehide handler been called. But given that there is simply no dump statement, I haven't seen the output. 

What we really should do here is to always let the code update the windows status. Even if we set loaded to false twice (here and in beforeunload) it will only get another guid. That doesn't hurt. But what we should not do is to always remove the event listener for beforeunload. Reason as I have mentioned above.

With that changed all should work fine, and I'm more inclined to give it a r+. The above code looks to dangerous to me, and I really don't want to see something else regressed.

Comment 28

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

This behavior can happen at any time for tests which are opening/closing tabs. So we have to fix that for mozmill 2.0.

Cosmin, it would be great if you could add two more testcases here beside the about:newtab one:

1. A content page which lands in the bfcache
2. A content page which has a beforeunload/unload handler and does not get cached

Seeing both of those passing should give us a high guarantee that nothing will go wrong.

Comment 29

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

(In reply to Henrik Skupin (:whimboo) from comment #28)
> Cosmin, it would be great if you could add two more testcases here beside
> the about:newtab one:
> 
> 1. A content page which lands in the bfcache
> 2. A content page which has a beforeunload/unload handler and does not get
> cached
> 
> Seeing both of those passing should give us a high guarantee that nothing
> will go wrong.
I added those cases in mutt test, and the page with unload handler fails to without the fix.

> What we really should do here is to always let the code update the windows
> status. Even if we set loaded to false twice (here and in beforeunload) it
> will only get another guid. That doesn't hurt. But what we should not do is
> to always remove the event listener for beforeunload. Reason as I have
> mentioned above.
So as you said, I removed the condition, and also the removeEventListener function, so it will always go through beforeUnloadHandler if the event rises after pagehide event.

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

I keep the condition, for removing the event handler removal

Comment 31

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

Comment on attachment 805928 [details] [diff] [review]
patch v3.0

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

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ -234,2 @@
>  
> -      aWindow.removeEventListener("beforeunload", beforeUnloadHandler, true);

I haven't said that this line has to be removed. We still have to keep it inside the if condition, so that we remove our beforeunload listener in case of non-cached pages. Otherwise this would cause a memory leak.

::: mutt/mutt/tests/data/notCached.html
@@ +1,1 @@
> +<!DOCTYPE html>

For the filename please use an underscore as separator like 'no_bfcache.html'.

@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +<head>
> +    <script type="text/javascript">
> +        window.addEventListener('unload', function () {}, false);

Don't use an inline function here. Also we have to unregister the handler when the page gets unloaded. Otherwise it would cause a memory leak.

::: mutt/mutt/tests/js/testUtils/testPageLoad.js
@@ +20,4 @@
>    // Container page
>    {url: BASE_URL + "iframe.html", type: "ID", value: "iframe"}
>  ];
> +const PAGE_EVENTS_DATA = [

You know what, lets create a new test file so we have all the reload cases handled separately. This file gets bigger and bigger and introducing a new TEST_DATA like config only brings in more confusion.

@@ +153,5 @@
>  
>  }
> +
> +var testReloadPageAfterTabChange = function () {
> +  PAGE_EVENTS_DATA.forEach(function (aLink) {

It's not a link but an URL.

Comment 32

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

Comment on attachment 805938 [details] [diff] [review]
patch v3.1

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

The other review comments are still open from my last review. Please fix those too.

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +230,2 @@
>      }
> +    if (aEvent.persisted)

Don't remove the comment. No-one will know later what's going on here. Instead make it more clear to say what should happen if persisted is true.

Comment 33

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.2

Comment 34

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

Comment on attachment 805947 [details] [diff] [review]
patch v3.2

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

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +231,5 @@
> +    // If we change tabs and come back and try to reload the page, the event will not
> +    // be persisted and beforeunload event will never fire so we update the page status
> +    // to false to change the uuid of page anyway, and we also want to remove the
> +    // beforeunload handler if the event is persisted and we are sure beforeunload
> +    // will never fire to remove the handler himself.

Sorry, but we don't want to have a novel for our comments. Please only do minor updates to the comment we had before that it covers the if condition and doesn't talk about the inverse situation.

::: mutt/mutt/tests/data/no_bfcache.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +    <head>
> +        <script type="text/javascript"  src="no_bfcache.js"></script>

Why are you adding an external js file? Lets keep the js code in that file.

::: mutt/mutt/tests/data/no_bfcache.js
@@ +1,2 @@
> +function UnloadHandler() {
> +    window.removeEventListener('unload', UnloadHandler, false);

Two blanks of indentation as usual and keep our naming convestions for function names.

::: mutt/mutt/tests/js/testUtils/testReloadPage.js
@@ +13,5 @@
> +  "about:newtab"
> +];
> +
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();

aModule please and also add 'use strict' at the beginning of the file.

Comment 35

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.3

I made the changes.
Thanks

Comment 36

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

Comment on attachment 806438 [details] [diff] [review]
patch v3.3

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

With the comment fixed for real, you will get my r+.

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +230,3 @@
>      }
> +    // If event.persisted is false, the beforeUnloadHandler should fire and will
> +    // remove the handler himself otherwise we have to remove it here

Please do what I have requested in my last review. The comment should not talk about the inverse situation. That's confusing.

Comment 37

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.4

I changed the comment, I'm very excited by seeing this landed !

Comment 38

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

Comment on attachment 807079 [details] [diff] [review]
patch v3.4

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

Looks good now. Please show me successful testruns for Nightly across platforms, and a run with esr17 on a single platform. For the latter we still have other problems, so I will be aware of. Also have you run the mutt tests with the different versions of Firefox?

Comment 39

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #38)
> Comment on attachment 807079 [details] [diff] [review]
> patch v3.4
> 
> Review of attachment 807079 [details] [diff] [review]:
>Also have you run the mutt tests with the different versions of Firefox?
Yes, mutt tests run perfect on all builds I tested so far.
The esr testrun was run with 915554 also applied for a clearer view.

Esr17
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c3af215
Nightly
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c3d519f
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c3ceacb
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c3d22b7

Comment 40

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

(In reply to Cosmin Malutan from comment #39)
> http://mozmill-crowd.blargon7.com/#/functional/report/
> 1039ea48a9d69a5a1cc4fd228c3d22b7

That one was run with Mozmill 1.5.22. But seeing that all others are passing, I'm confident that we are fine here. I will get it landed. Thanks!

Comment 41

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

Comment on attachment 807079 [details] [diff] [review]
patch v3.4

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

I had to fix a white-space error and update the commit message so it reflects what we are really fixing here. Please make sure in the future that all that has been done by yourself.

Comment 42

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

Landed as:
https://github.com/mozilla/mozmill/commit/05f859f618fb89f789d0c12fd1574f09e98fa0e9 (master)
https://github.com/mozilla/mozmill/commit/be7251b33264e8fa149b15d38d7960985b0a54c7 (hotfix-2.0)