686030 - When clicking on an element waitForPageLoad fails for target page

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

Attachment: Testcase

This is a regression from Mozmill 1.5. I would suspect it has been caused by the refactoring of init.js by Andrew a while back. Just execute the attached testcase with a recent Mozmill 2.0 dev build and it will fail.

Comment 1

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

This testcase is part of the test_waitForPageLoad.js test, which I will disable for now with my patch on bug 685865.

Comment 2

[Andrew Halberstadt [:ahal]]

So I don't think this is a regression, but rather a timing issue.  In order to tell whether or not the page has loaded we attach a property to the document called 'mozmillDocumentLoaded' (see https://github.com/mozautomation/mozmill/blob/5c9f9b69f1d0c0d155008be13e9894f4ce645bb8/mozmill/mozmill/extension/resource/modules/controller.js#L378).

Now when we call a function such as controller.open(), the page is reloaded and that property is gone so we can check that the new page loads, no problem.  In the attached test case, however, we are clicking an element which then causes the page to load.  What is likely happening is that mozmill is checking for the 'mozmillDocumentLoaded' property before the new page has even started loading, it finds the property and says 'ok, the document is loaded' and then returns.

This is kind of tricky to fix, a small sleep in waitForPageLoad would likely do the trick for 99% of the cases, but what if clicking a button executes some JS first, and *then* causes the page to load. We could never be sure that the sleep is long enough (not to mention the added slowness this would cause).

Any suggestions?

Comment 3

[Andrew Halberstadt [:ahal]]

Another possible solution is to have waitForPageLoad() remove the 'mozmillDocumentLoaded' property after it has finished waiting. I tried this and it fixes the above testcase, but the downside is that it will only work if waitForPageLoad() was called previously.  Also it could possibly break other things that may rely on that property being present.

Comment 4

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

The thing is that I was never able to reproduce this failure with Mozmill 1.5.x. It only happens with Mozmill 2.0. Is that not the case for you Andrew?

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Another possible solution is to have waitForPageLoad() remove the
> 'mozmillDocumentLoaded' property after it has finished waiting. I tried this
> and it fixes the above testcase, but the downside is that it will only work
> if waitForPageLoad() was called previously.  Also it could possibly break
> other things that may rely on that property being present.

There is nothing else which relies on it. But instead of removing it, we should set it to false instead. For me it sounds like a good idea.

Comment 5

[cmtalbert]

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Another possible solution is to have waitForPageLoad() remove the
> 'mozmillDocumentLoaded' property after it has finished waiting. I tried this
> and it fixes the above testcase, but the downside is that it will only work
> if waitForPageLoad() was called previously.  Also it could possibly break
> other things that may rely on that property being present.

Nothing else relies on it.  So I think this is a good solution.  Doesn't matter to me if we remove the attribute or set it to false, it's really the same as any other DOM attribute - it only matters if it is present and assigned the value of true.

In response to comment 4 and 1.5.x...
The entire page loading logic is different between 1.5.x and 2.x so if we're hunting a race condition, then that's going to manifest differently between the two products.  

The page loading logic was changed to better load for iframes etc, so it could be that the more complicated logic is exposing a race condition we've had all along that just wasn't exposed to us previously. I don't think rolling back to 1.5 style page loading is a viable alternative at this point, so I'm not sure why whether or not this happens on 1.5.x is an important point.

Comment 6

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

All changes I have made are identical between 1.5 and 2.0, including iframes and others. The only difference I can see are the changes to the event handlers made by Andrew. Would be easy to check by just replacing the code on master with 1.5.4 inside init.js.

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Remove mozmillDocumentLoaded property after load

Maybe there's a timing difference between when the load and pageshow events are sent. In either case, the patch to the event handlers didn't cause this bug, although it may have exposed it.

This patch removes the 'mozmillDocumentLoaded' property after loading. I'd rather remove the attribute completely than set it to false (setting the load property to false even though the document has loaded just feels wrong).

I'm still not entirely convinced this is the right way to go, but I guess the only other alternative is to have the test itself take care of this scenario.  The reason I hesitate is because a test could potentially call controller.isLoaded() and get back a false answer.  At the very least if we do go ahead with this we should take isLoaded() out of the controller so that it isn't accessible to tests.

Comment 8

[Andrew Halberstadt [:ahal]]

I did a grep of the mozmill-tests and controller.isLoaded is also used in lib/modal-dialog.js. Henrik, would you know of any other places it may be getting used?

I'm going to try to think of a way to fix this without breaking controller.isLoaded()

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 - Fix waitForPageLoad timing issue

Rather than remove the mozmillDocumentLoaded property, I add an additional property called mozmillWaitForPageLoad. This ensures isLoaded() will remain valid.

waitForPageLoad now uses both these properties to differentiate between the page being finished loading and the page not even having started loading. A by-product of this is that calling waitForPageLoad after the page has loaded will fail.  E.g this results in a failure:
controller.open('google.com');
controller.sleep(5000);
controller.waitForPageLoad(); 

I re-enabled the broken test in test_waitForPageLoad and I disabled the test that tests the above case.

Comment 10

[cmtalbert]

(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Created attachment 564200 [details] [diff] [review] [diff] [details] [review]
> Patch 2.0 - Fix waitForPageLoad timing issue
> 
> Rather than remove the mozmillDocumentLoaded property, I add an additional
> property called mozmillWaitForPageLoad. This ensures isLoaded() will remain
> valid.
> 
> waitForPageLoad now uses both these properties to differentiate between the
> page being finished loading and the page not even having started loading. A
> by-product of this is that calling waitForPageLoad after the page has loaded
> will fail.  E.g this results in a failure:
> controller.open('google.com');
> controller.sleep(5000);
> controller.waitForPageLoad(); 
Does this mean that we now have a race condition between when you load a page and call waitforpageload?  For example, if a page loads incredibly fast could the page complete loading between your click and the waitForPageLoad call such that you end up failing the waitForPageLoad call?

I'd rather that we find a way to make it not fail if the page loads before waitForPageLoad finishes.  Ideally, in that case it shouldn't wait at all - it should just turn around and say "done".

Comment 11

[Andrew Halberstadt [:ahal]]

Actually I lied, the above test case passes, it'll only fail if waitForPageLoad was previously called:
controller.open('google.com');
controller.waitForPageLoad();
controller.waitForPageLoad();

If the page load finishes really fast then the mozmillWaitForPageLoad property won't exist, and the mozmillDocumentLoaded property will so everything will work.

Comment 12

[cmtalbert]

(In reply to Andrew Halberstadt [:ahal] from comment #11)
> Actually I lied, the above test case passes, it'll only fail if
> waitForPageLoad was previously called:
> controller.open('google.com');
> controller.waitForPageLoad();
> controller.waitForPageLoad();
> 
> If the page load finishes really fast then the mozmillWaitForPageLoad
> property won't exist, and the mozmillDocumentLoaded property will so
> everything will work.

Ok, I like that.  Sounds great.  Thanks Andrew.

Comment 13

[Andrew Halberstadt [:ahal]]

master: https://github.com/mozautomation/mozmill/commit/0b4c4deb646df5615e86dac1d1019329d6c757d2