1475264 - Disable new evaluate.isCyclic check from bug 1447977

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

https://bugzilla.mozilla.org/show_bug.cgi?id=1447977 introduced a
new implementation of evaluate.isCyclic that was not based on
JSON.stringify to overcome cyclic references stored on Element
prototypes.

We suspect this may have caused an intermittency spike recorded in
https://bugzilla.mozilla.org/show_bug.cgi?id=1414495#c211 (although
probably unrelated to the actual bug itself).

This bug provides a patch which reverts this behaviour without
backing out the original changeset.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Temporarily disable new cylical check in Marionette. r=me, a=Aryx

Bug 1447977 introduced a new implementation of evaluate.isCyclic
that was not based on JSON.stringify to overcome cyclic references
stored on Element prototypes.

This patch reverts the new behaviour and hides it behind an environment
variable MOZ_MARIONETTE_NEW_CYCLIC.  This is useful so we can more
easily test the difference in automation.

MozReview-Commit-ID: 2xtNL596Imc

Comment 2

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a84917f7bb05003b393148e2580bfbf24c20744

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Temporarily disable new cylical check in Marionette. r=me, a=Aryx

Bug 1447977 introduced a new implementation of evaluate.isCyclic
that was not based on JSON.stringify to overcome cyclic references
stored on Element prototypes.

This patch reverts the new behaviour and hides it behind an environment
variable MOZ_MARIONETTE_NEW_CYCLIC.  This is useful so we can more
easily test the difference in automation.

MozReview-Commit-ID: 2xtNL596Imc

Comment 4

[Andreas Tolfsen ❲:ato❳]

Did you make any further discoveries regarding
https://bugzilla.mozilla.org/show_bug.cgi?id=1414495#c211?

I wonder if we should land this patch temporarily to see if it
reduces the OF over the next week?

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8991646 [details] [diff] [review]
Temporarily disable new cylical check in Marionette. r=me, a=Aryx

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

(In reply to Andreas Tolfsen 「:ato」 from comment #4)
> Did you make any further discoveries regarding
> https://bugzilla.mozilla.org/show_bug.cgi?id=1414495#c211?
No.

> I wonder if we should land this patch temporarily to see if it
> reduces the OF over the next week?
Yes, please give it a try.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Thanks.

Next merge date is in ~two months so this seems like a good time
to experiment.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7334a57640f
Temporarily disable new cylical check in Marionette. r=me, a=Aryx

Comment 8

[Tiberius Oros[:tiberius_oros]]

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

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/12f42d129762
Backed out changeset b7334a57640f at ato's request for causing spiking intermittents. a=backout

Comment 11

[Cristian Brindusan [:cbrindusan]]

Backed out changeset b7334a57640f (bug 1475264) at ato's request for causing spiking intermittents.

Backout: https://hg.mozilla.org/mozilla-central/rev/12f42d129762967c3f912085787ad489ec97feaa

Comment 12

[Andreas Tolfsen ❲:ato❳]

What failed?  Can you provide a link to the Treeherder job?

Comment 13

[Cristian Brindusan [:cbrindusan]]

This backout was made at your request (on IRC/developers), so I'm unable to provide more details.

"ato> CosminS|sheriffduty, CristianB|sheriffduty: Can either of you back out https://bugzilla.mozilla.org/show_bug.cgi?id=1475264?"

Comment 14

[Andreas Tolfsen ❲:ato❳]

Oh sorry, I was confused by the fact that I thought it was a backout
of the backout.  I didn’t expect a needinfo on this.  All’s good!

Comment 15

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

So we can close as wontfix now?