810376 - switch to frame should return once the readystate is complete

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

Right now, we switch to the frame immediately instead of returning once we check that the document's readyState is set to "complete".

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

We saw this when running today's Unagi build against the camera test: http://qa-selenium.mv.mozilla.com:8080/job/b2g.unagi.gaia/18/console.

The test ran, finished, and ran all the other tests without ever leaving the Camera app, because of this, we think.

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

13:51 stephend: mdas: does the test just need reworking?
13:51 mdas: when we listen for document ready, we return when the about:blank page is ready
13:51 mdas: for marionette, it thinks it's doing the right thing
13:52 mdas: stephend: I think so. what you can do is wait until marionette.get_url() returns the expected url
13:52 mdas: so marionette attaches itself to the right frame, and is running there, but that frame is just *really slow* to load

Comment 3

[Zac C (:zac)]

Are you sure that Marionette should not queue the switch_to_frame command until the event? What is the WebDriver behaviour?

The other problem I have with just doing an extra wait in the test is that this definitely 'regressed' on Thursday when I first mentioned it to mdas. I definitely have never seen this before the 8th November but since then I have seen it on more apps than just the Camera app. Maybe 1 in 4 app launches are affected.

Whether it was B2G or Marionette I am not sure but if you go back to the 5th or 6th build I'm sure you won't see this problem.

Comment 4

[Jonathan Griffin (:jgriffin)]

The difficulty here is that when loading an app iframe, about:blank gets loaded first, and then the app.  Depending on timing, Marionette can detect that the doc is ready for "about:blank" rather than the app which will get loaded after that.

I'd be surprised if WebDriver does anything to handle this.  Do you know if it does, David?

The reason this regressed is likely bug 807478; this had the effect of speeding up B2G somewhat, so we're more likely to hit the state where Marionette is detecting document ready for "about:blank".

Comment 5

[Jonathan Griffin (:jgriffin)]

I think the right solution is not to modify core Marionette, but to update the launch function to wait until the frame src is as expected:

https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_apps.js#L145

Comment 6

[Zac C (:zac)]

OK thanks, I understand the two-event scenario.

I found this in the WD source:
http://code.google.com/searchframe#2tHw6m3DZzo/trunk/javascript/firefox-driver/js/webLoadingListener.js&q=about:blank%20package:selenium\.googlecode\.com&type=cs&l=145

I'm :automatedtester can elaborate a bit more.

Comment 7

[Zac C (:zac)]

Also I'd rather fix this in Marionette Core because an app can (and will in future tests) be launched from outside the context of the launch function, for example a touch on the icon or launched by another app.

Comment 8

[Jonathan Griffin (:jgriffin)]

We have something similar in Marionette:  http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#529

But, this is called after go_url(), not switch_to_frame(), and Gaia apps are launched without using go_url().

Comment 9

[David Burns :automatedtester]

-The following is how WebDriver does it-

We *try* wait for all the iframes to have loaded. The default is to do this. The link that Zac showed was for our experimental listener that returns early but to be honest all of this is black magic while we try solve the Halting Problem[1] 

We use nsIWebProgressListener to handle this and then listen to what is happening. This, I must note, only happens when we do a click and when we do a driver.get() which is equivalent to marionette.navigate().

so... should switch_to_frame wait for a page to load. No. Should we TRY and watch for a new iframe and wait for it to be loaded on a touch/click. Yes.

That will get us on a par with WebDriver. 

If you want to see our loading listeners they can be found at https://code.google.com/p/selenium/source/browse/trunk/javascript/firefox-driver/js/webLoadingListener.js

I hope that makes sense


[1] http://en.wikipedia.org/wiki/Halting_problem

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: patch for listeners v1.0

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: patch for actors v1.0

I'm currently testing this patch under try since my build system is borked:

https://tbpl.mozilla.org/?tree=Try&rev=af63f4c9d7b3

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 681214 [details]
patch for actors v1.0

Not sure how to remove this patch. This doesn't really work in chrome yet, it hangs on desktop.

Comment 13

[Jonathan Griffin (:jgriffin)]

Comment on attachment 681213 [details] [diff] [review]
patch for listeners v1.0

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

I'm not sure we need this code in marionette-actors; it certainly isn't needed for B2G.  If it's hard to implement there, I'd say just punt on it for now.

::: testing/marionette/marionette-listener.js
@@ +804,5 @@
> +      sendOk();
> +      return;
> +    } 
> +    checkTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);
> +  }

This will cause a socket.timeout if there's an error loading the page (i.e., the wifi is off).  We should catch this case with code something like http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#533, so that if we hit an error state we still sendOk(), so that the script can continue.

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 681213 [details] [diff] [review]
patch for listeners v1.0

Whoops, missed the +

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Listener patch pushed to ash:
https://tbpl.mozilla.org/?tree=Ash&rev=31b811b620ea

Listener patch pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=e450947b007b

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Jonathan Griffin (:jgriffin) from comment #13)
> Comment on attachment 681213 [details] [diff] [review]
> patch for listeners v1.0
> 
> Review of attachment 681213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure we need this code in marionette-actors; it certainly isn't
> needed for B2G.  If it's hard to implement there, I'd say just punt on it
> for now.
> 
> ::: testing/marionette/marionette-listener.js
> @@ +804,5 @@
> > +      sendOk();
> > +      return;
> > +    } 
> > +    checkTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);
> > +  }
> 
> This will cause a socket.timeout if there's an error loading the page (i.e.,
> the wifi is off).  We should catch this case with code something like
> http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> listener.js#533, so that if we hit an error state we still sendOk(), so that
> the script can continue.

Arg, I missed this comment and only saw the r+ comment! Do we want to sendOk(), or sendError()? If the iframe doesn't load, we should warn the user, no?

In any case, in light of David's comments, should we still land this, or should we focus on a solution that would give us parity with webdriver? A parity solution seems to be a bit tricky in this case, since the iframe event listener has to be added to the system app in b2g (since clicking the camera icon will create an OOP frame), which is different behaviour from desktop...

Comment 17

[Jonathan Griffin (:jgriffin)]

(In reply to Malini Das [:mdas] from comment #16)
> 
> Arg, I missed this comment and only saw the r+ comment! Do we want to
> sendOk(), or sendError()? If the iframe doesn't load, we should warn the
> user, no?
> 

I guess we should sendError().  I'm trying to think if this would cause any unintended consequences, but can't see any.

> In any case, in light of David's comments, should we still land this, or
> should we focus on a solution that would give us parity with webdriver? A
> parity solution seems to be a bit tricky in this case, since the iframe
> event listener has to be added to the system app in b2g (since clicking the
> camera icon will create an OOP frame), which is different behaviour from
> desktop...

WebDriver parity will not solve Zac's problem, since WebDriver only attempts to listen for frame loads on navigate/click.  I don't think this patch will break WebDriver compatibility, AFAICT, so I think we should still land it, unless David feels strongly otherwise.

Comment 18

[Malini Das [:mdas] - Away, not checking bugmail]

Landed the modified content patch (with sendError) on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/764eee3ba0f1

Landed it on aurora as well:
https://hg.mozilla.org/releases/mozilla-aurora/rev/866bbd219b0e

Comment 19

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: patch for actors v2.0

A very similar patch for chrome. Tested it against m-c and it has passed all tests

Comment 20

[Jonathan Griffin (:jgriffin)]

Comment on attachment 681589 [details] [diff] [review]
patch for actors v2.0

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

cool, looks good

Comment 21

[Malini Das [:mdas] - Away, not checking bugmail]

actor patch on try:
https://tbpl.mozilla.org/?tree=Try&rev=70fa1422c859

actor patch on ash:
https://tbpl.mozilla.org/?tree=Ash&rev=f648e9c0e88e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/764eee3ba0f1

Comment 23

[Malini Das [:mdas] - Away, not checking bugmail]

pushed actors patch to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e320680de4fc

pushed actors patch to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e774e4c207dc

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e320680de4fc