Closed Bug 810376 Opened 7 years ago Closed 7 years ago

switch to frame should return once the readystate is complete

Categories

(Testing :: Marionette, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla19

People

(Reporter: mdas, Assigned: mdas)

Details

Attachments

(2 files, 1 obsolete file)

Right now, we switch to the frame immediately instead of returning once we check that the document's readyState is set to "complete".
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.
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
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.
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".
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
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.
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.
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().
-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
Attached file patch for actors v1.0 (obsolete) —
I'm currently testing this patch under try since my build system is borked:

https://tbpl.mozilla.org/?tree=Try&rev=af63f4c9d7b3
Attachment #681214 - Flags: review?(jgriffin)
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.
Attachment #681214 - Flags: review?(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 on attachment 681213 [details] [diff] [review]
patch for listeners v1.0

Whoops, missed the +
Attachment #681213 - Flags: review?(jgriffin) → review+
(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...
(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.
A very similar patch for chrome. Tested it against m-c and it has passed all tests
Attachment #681214 - Attachment is obsolete: true
Attachment #681589 - Flags: review?(jgriffin)
Attachment #681589 - Attachment is patch: true
Comment on attachment 681589 [details] [diff] [review]
patch for actors v2.0

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

cool, looks good
Attachment #681589 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/764eee3ba0f1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.