1685454 - Enhance retry logic for getMarionetteCommandsActorProxy for inactive actors.

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

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

With bug 1669961 fixed (already in Firefox 84) we should basically not use this.document.defaultView in the JSWindowActor child instances, but this.contentWindow instead. Reason is that this.document.defaultView can actually return a value even when the window isn't active anymore like window proxy transplanting. As such invalid data can be returned. Maybe this is the reason for bug 1661591. Not sure about the failures for getCurrentURL (bug 1664881) yet.

This means we basically revert the work from bug 1670332, including instances of this.document.defaultView that were added afterward.

Comment 1

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

Attachment: Bug 1685454 - [marionette] Use "this.contentWindow" instead of "this.document.defaultView" in JSWindowActor child instances.

Comment 2

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

As discussed on Matrix we actually have concerns in going back to use this.contentWindow everywhere. Reason is that certain code rely on the window proxy the current JSWindowActor instance is bound to. Also when working with elements a good chunk of code (including imported modules) makes use of element.ownerDocument.defaultView, which will certainly create a discrepancy here.

So maybe a check for this.contentWindow === null needs to be done before any code in the child actor can be executed. Throwing a DOMException of type InactiveWindow under such a situation, our retry logic in the wrapper of the parent actor could automatically kick in, by finding the current actor and resending the command. Local testing indicates that this at least works fine.

Would it maybe make sense for the JSWindowActor implementation to have a method similar to actorDestroyed but just for the case when the actor / window gets inactive (eg. page moved to bfcache)? That way implementations could run specific code to handle such a situation. And maybe another DOMException beside AbortError could be thrown by JSWindowActor itself?

Comment 3

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

Nika, can you please have a look at the former comment? Thanks.

Comment 4

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

Attachment: Bug 1685454 - [marionette] Enhance retry logic for getMarionetteCommandsActorProxy for inactive actors.

When the window of the current child actor instance is moved
into bfcache the actor doesn't get destroyed. Instead
this.contentWindow will become "null", which means that the
command has to be run again in the active actor instance.

By raising a specific error message in the child actor, the
commands actor proxy can handle that error for its retry logic.

Comment 5

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

The patch from the last comment shows the approach that works fine for me locally.

Comment 6

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

I'm going to land this patch, but would still be interested to hear from Nika if that approach is fine. If not, I will file a follow-up bug for required changes.

Comment 7

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

Also it's not per se a regression from bug 1670332, but just a missing check that can cause issues when running WebDriver commands while Firefox is navigating. As such it would still be good to get this uplifted to 85.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c049864e6185
[marionette] Enhance retry logic for getMarionetteCommandsActorProxy for inactive actors. r=marionette-reviewers,jdescottes

Comment 9

[Alexandru Michis [:malexandru]]

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

Comment 10

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

Note that we should update the 84 release notes to mention this specific known bug:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/84#webdriver_conformance_marionette

I will do that today.

Comment 12

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

Comment on attachment 9195923 [details]
Bug 1685454 - [marionette] Enhance retry logic for getMarionetteCommandsActorProxy for inactive actors.

Beta/Release Uplift Approval Request

• User impact if declined: WebDriver tests have a relatively high likelihood to fail when trying checking for stale elements after a page navigation.

Even with a workaround by setting marionette.actors.enabled to false we would really like to keep users with our Fission implementation running.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds a null check, which triggers retrying the specific command.
• String changes made/needed:

Comment 13

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #12)

Beta/Release Uplift Approval Request

• User impact if declined: WebDriver tests have a relatively high likelihood to fail when trying checking for stale elements after a page navigation.

Even with a workaround by setting marionette.actors.enabled to false we would really like to keep users with our Fission implementation running.

Sorry, but that was the text I wanted to add for bug 1684827. In this case we have an inactive actor, which actually should not run any WebDriver code. Instead a new JSWindowActor pair has to be requested. As workaround marionette.actors.enabled can be set to false, but we would really like to keep users with our Fission implementation running.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #2)

Would it maybe make sense for the JSWindowActor implementation to have a method similar to actorDestroyed but just for the case when the actor / window gets inactive (eg. page moved to bfcache)? That way implementations could run specific code to handle such a situation.

We could potentially add a set of other callbacks, like pagehide and pageshow, though it would also be possible to add JS listeners for this yourself as well, iirc.

And maybe another DOMException beside AbortError could be thrown by JSWindowActor itself?

I'm not sure what exception you'd want to be thrown in this situation. I worry we'd break code if we started making calls like sendAsyncMessage throw when they didn't previously.

Comment 15

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

(In reply to Nika Layzell [:nika] (ni? for response) from comment #14)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #2)

Would it maybe make sense for the JSWindowActor implementation to have a method similar to actorDestroyed but just for the case when the actor / window gets inactive (eg. page moved to bfcache)? That way implementations could run specific code to handle such a situation.

We could potentially add a set of other callbacks, like pagehide and pageshow, though it would also be possible to add JS listeners for this yourself as well, iirc.

Thanks Nika. I filed bug 1686175 for that. We will have a look if that might improve our inactive actor detection.

And maybe another DOMException beside AbortError could be thrown by JSWindowActor itself?

I'm not sure what exception you'd want to be thrown in this situation. I worry we'd break code if we started making calls like sendAsyncMessage throw when they didn't previously.

I'm happy to restrict this behavior to our actor implementation. As it looks like it works fine right now.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9195923 [details]
Bug 1685454 - [marionette] Enhance retry logic for getMarionetteCommandsActorProxy for inactive actors.

approved for 85.0b8

Comment 17

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33ae419c2556