860849 - MarionetteException: Illegal value in CI from test_clock_add_alarm_multiple_times run

Status: VERIFIED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

I don't know how reproducible this is, but we saw it in CI:

======================================================================
ERROR: test_clock_add_alarm_multiple_times (test_clock_add_alarm_multiple_times.TestClockAddAlarmMultipleTimes)
Add multiple alarm
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/jenkins/workspace/b2g.unagi.gaia.nightly.ui/gaiatest/tests/clock/test_clock_add_alarm_multiple_times.py", line 40, in test_clock_add_alarm_multiple_times
    self.assertEqual(i, len(self.clock.alarms))
  File "/var/jenkins/workspace/b2g.unagi.gaia.nightly.ui/gaiatest/apps/clock/app.py", line 47, in alarms
    return [self.Alarm(self.marionette, alarm) for alarm in self.marionette.find_elements(*self._all_alarms_locator)]
  File "/var/jenkins/workspace/b2g.unagi.gaia.nightly.ui/.env/local/lib/python2.7/site-packages/marionette_client-0.5.23-py2.7.egg/marionette/marionette.py", line 591, in find_elements
    response = self._send_message('findElements', 'value', **kwargs)
  File "/var/jenkins/workspace/b2g.unagi.gaia.nightly.ui/.env/local/lib/python2.7/site-packages/marionette_client-0.5.23-py2.7.egg/marionette/marionette.py", line 310, in _send_message
    self._handle_error(response)
  File "/var/jenkins/workspace/b2g.unagi.gaia.nightly.ui/.env/local/lib/python2.7/site-packages/marionette_client-0.5.23-py2.7.egg/marionette/marionette.py", line 367, in _handle_error
    raise MarionetteException(message=message, status=status, stacktrace=stacktrace)
TEST-UNEXPECTED-FAIL | test_clock_add_alarm_multiple_times.py TestClockAddAlarmMultipleTimes.test_clock_add_alarm_multiple_times | MarionetteException: Illegal value
----------------------------------------------------------------------
Ran 1 test in 53.035s

Will grab the logcat and attach, once the run is done; wanted to get this on file, first.

Test: https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/tests/clock/test_clock_add_alarm_multiple_times.py
App object: https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/apps/clock/app.py#L18

    _all_alarms_locator = ('css selector', '#alarms li')

Comment 1

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

Run was: http://qa-selenium.mv.mozilla.com:8080/view/B2G/job/b2g.unagi.gaia.nightly.ui/194/console

Comment 2

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

Attachment: logcat

Comment 3

[David Burns :automatedtester]

I noticed this on desktop too while trying to replicate bug 838607 so makes me suspect a change to transport layer

Comment 4

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

Malini, mind taking a look at this tomorrow, along with bug 860854?  Both are hitting our automation quite hard (i.e. affecting more tests than I've linked here); thx!

Comment 5

[Bob Silverberg [:bsilverberg]]

I just saw this error with a run of gaiatest/tests/contacts/test_sort_contacts.py. I fixed a locator in that test and ran it several times. The first 3 passed but the last one failed with the MarionetteException: Illegal value error. Trace is at https://gist.github.com/bobsilverberg/5392338.

Comment 6

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

This is due to a bug introduced by bug 736592. If we retrieve an element that's been GC'd, we still try to call XPCNativeWrapper on it and that throws this error. A patch will be up soon.

Comment 7

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

Attachment: check element before wrapping

Running patch through try first https://tbpl.mozilla.org/?tree=Try&rev=67f1971185ad

I tested this patch locally against the given gaia-ui-test here and it passes.

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 738065 [details] [diff] [review]
check element before wrapping

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

Some drive-by comments.

::: testing/marionette/marionette-elements.js
@@ +71,4 @@
>    */
>    addToKnownElements: function EM_addToKnownElements(element) {
>      for (let i in this.seenItems) {
> +      let foundEl = this.seenItems[i].get();

Should we remove this.seenItems[i] if !foundEl?

@@ -140,5 @@
> -          for(let i in this.seenItems) {
> -            if (this.seenItems[i].get() == val) {
> -              result = {'ELEMENT': i};
> -            }
> -          }

haha, good catch

Comment 9

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

Comment on attachment 738065 [details] [diff] [review]
check element before wrapping

seems to be going green on try

Comment 10

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

(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> Comment on attachment 738065 [details] [diff] [review]
> check element before wrapping
> 
> Review of attachment 738065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some drive-by comments.
> 
> ::: testing/marionette/marionette-elements.js
> @@ +71,4 @@
> >    */
> >    addToKnownElements: function EM_addToKnownElements(element) {
> >      for (let i in this.seenItems) {
> > +      let foundEl = this.seenItems[i].get();
> 
> Should we remove this.seenItems[i] if !foundEl?
> 

Sorry, I missed your comments. I'll change that

Comment 11

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

Not sure how I managed to change the severity, there, but let's put that back...

Also, I'm trying a patch with the foundEl removal change in it on try: https://tbpl.mozilla.org/?tree=Try&rev=8ad692a58ba1

Comment 12

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

Attachment: check element before wrapping v2

Comment 13

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

Comment on attachment 738127 [details] [diff] [review]
check element before wrapping v2

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

Bah, tested and uploaded the a non-refreshed patch. Don't review yet.

Comment 14

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

Attachment: check element before wrapping v3

fixed error and verified this patch locally against the alarm test.

Testing in try: https://tbpl.mozilla.org/?tree=Try&rev=91abc5d2729a

Comment 15

[Jonathan Griffin (:jgriffin)]

Comment on attachment 738180 [details] [diff] [review]
check element before wrapping v3

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

Thanks mdas.

Comment 16

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

pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6600519161

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b1d335593453

Comment 19

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

I kicked off a subsequent Unagi engineering build which had this fix, and it's verified FIXED with:

<!-- Mercurial-Information: <project name="releases/mozilla-b2g18" path="gecko" remote="hgmozillaorg" revision="b1d335593453"/> -->
  <!-- Mercurial-Information: <project name="integration/gaia-v1-train" path="gaia" remote="hgmozillaorg" revision="171adc0c8ca1"/> -->

Comment 20

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

Verified FIXED; thx.

Comment 21

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/625540982229