605784 - Event listener "pageshow" for tabs let waitForPageLoad abort too early

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

+++ This bug was initially created as a clone of Bug #604878 +++

With the fix on bug 604878 we can now exactly wait for the page loaded event for each individual tab. The only downside is, that it doesn't work with bfcache yet. To make that work the "pageshow" event can be processed instead of the "load" event.

I had already implemented that solution but a couple of tests failed afterward. It mainly happened when a new tab gets created and a page is directly loaded.

It should be investigated and I would like to have this feature in Mozmill-1.5.2.

Comment 1

[cmtalbert]

Did you have time to do this?  I really hesitate to touch this code as it is very regression prone.  Do we have a demonstrated need for this in 1.5.2?

Comment 2

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

No, lets skip this. There is no pressure on it. In such a case we would have to use waitForElement instead.

Comment 3

[cmtalbert]

Is this really necessary for 2.0?  What does this enable you to do that you can't now?  Can you attach a testcase that clearly and succintly demonstrates the problem?

Comment 4

[Aaron Train [:aaronmt]]

Ran into this yesterday, ex (http://hg.mozilla.org/qa/mozmill-tests/rev/b24479a1b338). One case was in checking for elements on a page that was loaded completely before private browsing. On exit of private browsing (bfcache), there was a waitForPageLoad that wasn't sufficient. Test needed a waitForElement

Comment 5

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

Clint, right now waitForPageLoad() doesn't reliable wait for the page to be loaded. So whenever we restore tabs (exit of private browsing mode, session restore, or the back and forward buttons) you cannot use waitForPageLoad but have to waitForElement.

Comment 6

[cmtalbert]

(In reply to comment #5)
> Clint, right now waitForPageLoad() doesn't reliable wait for the page to be
> loaded. So whenever we restore tabs (exit of private browsing mode, session
> restore, or the back and forward buttons) you cannot use waitForPageLoad but
> have to waitForElement.
Ok, that makes it more clear to me.  Thanks.

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: WIP Patch v1

I came up with this patch which seems to work. 
All I can say is that:
1) I created a test case that fails without this patch, and passes with it.
2) It doesn't break any of our mutt tests.

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: Test case which fails without above patch, but passes with it

Henrik, do you remember which tests started failing when you tried doing this? The above patch seems to work, but I'm not sure how to properly test it.

Comment 9

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

No sorry. But you should run your patch with all of our tests in default. All tests should pass, if one fails you know it has been regressed something.

Comment 10

[Andrew Halberstadt [:ahal]]

I'll do that and that should be good for regressions, but what about testing whether or not this bug is actually fixed?

I can say that my attached test passes, but I don't know if this is actually solving the problem. Could you provide some more specific STR?

Comment 11

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

The back and forward buttons are the best proof of the patch. Also a normal reload should fetch the data from the cache.

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 523442 [details] [diff] [review]
WIP Patch v1

I'm going to go ahead and flag this for review. It doesn't break anything that I've tested and waitForPageLoad now works when using the browser back/forward buttons.

Comment 13

[Heather Arthur [:harth]]

Comment on attachment 523442 [details] [diff] [review]
WIP Patch v1

Review of attachment 523442 [details] [diff] [review]:

Looks good.

Comment 14

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

We really need at least one test here. I'm a bit worried that in the current state we can't use our existent Mozmill tests with the version on master to make sure we don't break anything.

Andrew, does this patch apply cleanly on hotfix-1.5 so you could simply execute a testrun against all of our tests - if not done yet?

Comment 15

[cmtalbert]

(In reply to comment #14)
> We really need at least one test here. I'm a bit worried that in the current
> state we can't use our existent Mozmill tests with the version on master to
> make sure we don't break anything.
> 
> Andrew, does this patch apply cleanly on hotfix-1.5 so you could simply execute
> a testrun against all of our tests - if not done yet?

There is a test attached to this bug and we will land that.  The test does fail without the patch and passes with the patch in place.

Comment 16

[Andrew Halberstadt [:ahal]]

master: https://github.com/mozautomation/mozmill/commit/776625521d639ce3f480fe0eb73a11e5f0abcd94

Checked in the test as well.