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

VERIFIED FIXED

Status

Testing Graveyard
Mozmill
--
major
VERIFIED FIXED
7 years ago
a year ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Dependency tree / graph

Details

(Whiteboard: [mozmill-1.5.1+])

Attachments

(2 attachments, 5 obsolete attachments)

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.
Summary: documentLoaded totally fails for tabs loaded in background because of it is shared and never gets set → Rework event listeners for "load" events so documentLoaded can be used for individual tabs
Created attachment 483742 [details] [diff] [review]
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!
Attachment #483742 - Flags: review?(ctalbert)
Attachment #483742 - Flags: feedback?(fayearthur+bugs)
Comment on attachment 483742 [details] [diff] [review]
Patch v1

Forgot to enable half of the tests. Now those have catched new failures.
Attachment #483742 - Attachment is obsolete: true
Attachment #483742 - Flags: review?(ctalbert)
Attachment #483742 - Flags: feedback?(fayearthur+bugs)
Created attachment 483760 [details] [diff] [review]
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.
Attachment #483760 - Flags: review?(ctalbert)
Attachment #483760 - Flags: feedback?(fayearthur+bugs)
Created attachment 483797 [details] [diff] [review]
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.
Attachment #483760 - Attachment is obsolete: true
Attachment #483797 - Flags: review?(ctalbert)
Attachment #483760 - Flags: review?(ctalbert)
Attachment #483760 - Flags: feedback?(fayearthur+bugs)

Comment 5

7 years ago
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.
Attachment #483797 - Flags: review?(ctalbert) → review-

Comment 6

7 years ago
Created attachment 483911 [details]
The show-all test run

Comment 7

7 years ago
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.
(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.
And further see the patch on bug 604832 which is necessary for our tests right after builds for 1.5.1 are available.
Created attachment 483923 [details] [diff] [review]
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.
Attachment #483797 - Attachment is obsolete: true
Attachment #483923 - Flags: review?(ctalbert)
And we need this patch for 1.5.1. Otherwise we will constantly fail when pages get loaded in background tabs.
Created attachment 483925 [details] [diff] [review]
Patch v2.1

Updated patch so it can be applied on top of the already landed thisObject patch.
Attachment #483923 - Attachment is obsolete: true
Attachment #483925 - Flags: review?(ctalbert)
Attachment #483923 - Flags: review?(ctalbert)
Created attachment 483927 [details] [diff] [review]
Patch v2.1

Now the correct patch for mozmill and not the one for the mozmill tests.
Attachment #483925 - Attachment is obsolete: true
Attachment #483927 - Flags: review?(ctalbert)
Attachment #483925 - Flags: review?(ctalbert)

Comment 14

7 years ago
(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

7 years ago
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.
Attachment #483927 - Flags: review?(ctalbert) → review+

Updated

7 years ago
Whiteboard: [mozmill-1.5.1?] → [mozmill-1.5.1+]

Comment 16

7 years ago
Landed: http://github.com/mozautomation/mozmill/commit/e95e7317584038a1caaa55877c226cf79e1cdbfd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-1.5.1+] → [mozmill-1.5.1+][needs-landing-2.0]
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
Blocks: 605784
Verified fixed (bustage fix above and this bug) on 1.5.1rc1
Status: RESOLVED → VERIFIED
Landed on master as:
http://github.com/mozautomation/mozmill/commit/c82be9f0e0fd61fb7910fe5f43349d9dfe57092a
Whiteboard: [mozmill-1.5.1+][needs-landing-2.0] → [mozmill-1.5.1+]
Depends on: 607973
Depends on: 610134
Depends on: 659000
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.