604878 - Rework event listeners for "load" events so documentLoaded can be used for individual tabs

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

Our code in init.js is totally crappy when you have to use the documentLoaded property.

http://mxr.mozilla.org/mozilla-central/source/testing/mozmill/mozmill-1.5.0/mozmill/extension/resource/modules/init.js

Here the reasons:

1. The documentLoaded property is shared between all tab instances. This is completely broken because we cannot use it for individual tabs. The property has to be bind to each tab instead.

2. We do not add "load" listeners for each open tab, but we only do that for the window itself. That's why only the selected tab receives the "load" event. Loading pages in background tabs will never trigger this event. We will have to add "load" event listeners for each open tab.

Comment 1

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

Attachment: Patch v1

I'm not sure why this code ever worked. Loading tabs in the background was broken since the early beginnings of this code. With this patch we can safely detect when pages have been loaded - and that tab independent!

Comment 2

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

Comment on attachment 483742 [details] [diff] [review]
Patch v1

Forgot to enable half of the tests. Now those have catched new failures.

Comment 3

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

Attachment: Patch v1.1

This patch fixes the remaining failures. I have did a test-run with all of our tests and it looks great.

Some of our existing tests will have to be updated to play nicely with this patch. I will cover that work on bug 604832.

Comment 4

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

Attachment: Patch v1.2

This patch includes the listener for "pageshow" which allows us to use waitForPageLoad even for pages served from the cache. As far as I can see that should be the final version before your review now.

Comment 5

[cmtalbert]

Comment on attachment 483797 [details] [diff] [review]
Patch v1.2

I'm not so sure about this patch. It's failing several tests in the set that we want to go live with for buildbot integration.  Specifically:
* testGoButton.js
* testSetToCurrentPage.js
* testUndoTabFromContextMenu.js
* testAccessPageInfoDialog.js
* testBackForwardButtons.js

Tested with latest nightly and latest checkout of qa/mozmill-tests on linux.

I'll attach a show-all test run.  I'll set about debugging this tomorrow.  If I don't see something simple to fix, then we may just need to hold off on this until 1.5.2.  The other problem here is that you're adding listeners to every single tab and never removing them.  I'm afraid that will leak.  But as I don't have a debug build handy, I'm building one now to debug and verify that this is in fact the case.

Comment 6

[cmtalbert]

Attachment: The show-all test run

Comment 7

[cmtalbert]

You know, I applied this on top of the changes I already landed for bug 604545.  I wonder if that's why I got different results than when you tested.  I'll look at it tomorrow, the debug build is off and running.

Comment 8

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

(In reply to comment #7)
> You know, I applied this on top of the changes I already landed for bug 604545.
>  I wonder if that's why I got different results than when you tested.  I'll
> look at it tomorrow, the debug build is off and running.

Oh, yes! That's the reason for. The changes to waitFor will cause problems here. I should have mentioned it. Sorry. That's sadly the reason when patches are waiting for review so long. I totally forgot about that. If you can review bug 604545 and we can land it, I can still come up with an updated patch here.

(In reply to comment #5)
> until 1.5.2.  The other problem here is that you're adding listeners to every
> single tab and never removing them.  I'm afraid that will leak.  But as I don't
> have a debug build handy, I'm building one now to debug and verify that this is
> in fact the case.

No, I don't add listeners to each single tab. Those which get added are for the window and it's done only once, specifically when a window gets opened. As what I was told, when a window gets closed all listeners will get removed automatically. So I'm not sure about how it is possible to leak those listeners.

Comment 9

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

And further see the patch on bug 604832 which is necessary for our tests right after builds for 1.5.1 are available.

Comment 10

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

Attachment: Patch v2

I did another test-run and had to update the patch a bit. First we shouldn't run a null check for a parameter which is a number. That will fail. Further I had to remove the lately added pageshow listener because it causes test failures. Not sure yet why but we shouldn't listen to that event for 1.5.1. It will need further investigation. I don't have the time this week.

Comment 11

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

And we need this patch for 1.5.1. Otherwise we will constantly fail when pages get loaded in background tabs.

Comment 12

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

Attachment: Patch v2.1

Updated patch so it can be applied on top of the already landed thisObject patch.

Comment 13

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

Attachment: Patch v2.1

Now the correct patch for mozmill and not the one for the mozmill tests.

Comment 14

[cmtalbert]

(In reply to comment #13)
> Created attachment 483927 [details] [diff] [review]
> Patch v2.1
> 
> Now the correct patch for mozmill and not the one for the mozmill tests.

Good news is that this patch version passes many of the tests.  The bad news is that it now fails:
* testAccessPageInfoDialog.js
* testStartStopPBMode.js
* testTabRestoration.js
* testBackForwardButtons.js

I don't think we can really contain the effort required to get this thing into a shippable state *this week*.  I'm really thinking of holding off on this patch until 1.5.2.  It causes way more problems than we have and we have plenty to deal with already.

Comment 15

[cmtalbert]

Comment on attachment 483927 [details] [diff] [review]
Patch v2.1

With the corresponding test changes, this looks good.  I am seeing that it doesn't solve all our problems with flaky tests unfortunately.  Testing on windows shows that there are still some tests that intermittently fail (passed on linux, failing on windows), namely:
In PrivateBrowsing tests:
* testStartStopPBMode.js
* testTabRestoration.js

But otherwise, the tests that are failing have not been modified to work with the new patch, so it looks good.

Thanks Henrik.

Comment 16

[cmtalbert]

Landed: http://github.com/mozautomation/mozmill/commit/e95e7317584038a1caaa55877c226cf79e1cdbfd

Comment 17

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

I had to push a bustage fix for waitForPageLoad which timed out after 5s per default. Simply slipped through the review:

http://github.com/mozautomation/mozmill/commit/f049743ed9c149fcf7b6021b19ade30f23b11b3c

Comment 18

[Aaron Train [:aaronmt]]

Verified fixed (bustage fix above and this bug) on 1.5.1rc1

Comment 19

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

Landed on master as:
http://github.com/mozautomation/mozmill/commit/c82be9f0e0fd61fb7910fe5f43349d9dfe57092a