1674172 - getMarionetteCommandsActorProxy doesn't check for a valid browsing context

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

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

Running all the Mn unit tests (here test_quit_restart.py) while having the upcoming patches for bug 1673823 applied I can see the following in the logs:

1603992368558	Marionette	DEBUG	2 -> [0,16,"WebDriver:ExecuteScript",{"script":"Components.utils.import(\"resource://gre/modules/Services.jsm\");\n            let ... ],"filename":"testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py","sandbox":"default","line":122}]
1603992368586	Marionette	TRACE	[16] Frame script unloaded
1603992368591	Marionette	DEBUG	2 <- [1,16,null,{"value":null}]
JavaScript error: resource://activity-stream/lib/ASRouter.jsm, line 969: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIObserverService.removeObserver]
console.error: Region.jsm: "Error fetching region" (new TypeError("NetworkError when attempting to fetch resource.", ""))
console.error: Region.jsm: "Failed to fetch region" (new Error("NO_RESULT", "resource://gre/modules/Region.jsm", 376))
1603992368600	Marionette	DEBUG	Closed connection 2
JavaScript error: chrome://marionette/content/actors/commands_parent.js, line 324: TypeError: can't access property "getActor", browsingContextFn().currentWindowGlobal is null

Note that those lines should also exist without my patches.

It means we currently fail to check for null when trying to get the currentWindowGlobal from the browsing context as returned by browsingContextFn().

Julian, mind having a look?

Comment 1

[Chris Peterson [:cpeterson]]

Tracking marionette-fission-mvp bugs for Fission Beta milestone (M7).

Comment 2

[Julian Descottes [:jdescottes]]

Missed the ni? looking at this right now. Keeping the ni? for now.

I can reproduce on central when running:

./mach test test_quit_restart.py --enable-fission -vv --gecko-log - 

We are hitting the issue which is alluded to in this comment:

              // TODO: Scenarios where the window/tab got closed and
              // currentWindowGlobal is null will be handled in Bug 1662808.
              const actor = browsingContextFn().currentWindowGlobal.getActor(
                "MarionetteFrame"
              );

https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/testing/marionette/actors/MarionetteFrameParent.jsm#333-338

Which we discussed a bit in the review at https://phabricator.services.mozilla.com/D94086?id=356370#inline-535100

Comment 3

[Julian Descottes [:jdescottes]]

Added a few logs, we get this issue when trying to use the "cleanUp":

  if (MarionettePrefs.useActors) {
    if (this.getBrowsingContext()) {
      try {
        // reset any global state used by parent actor
        this.getActor().cleanUp();
      } catch (e) {
        if (e.result != Cr.NS_ERROR_DOM_NOT_FOUND_ERR) {
          throw e;
        }
      }
    }
    ChromeUtils.unregisterWindowActor("MarionetteFrame");
  }

https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/testing/marionette/driver.js#3020-3032

And the cleanUp implementation in the actor:

  cleanUp() {
    elementIdCache.clear();
  }

We can look at this in different ways:

• cleanUp doesn't depend on the ParentActor instance. elementIdCache is a singleton in the scope of MarionetteFrameParent.jsm -> we could just expose cleanUp outside of the actor
• driver.js checks this.getBrowsingContext() but doesn't check for the window, which is mandatory to get the actor -> we could add such a check
• we could throw Cr.NS_ERROR_DOM_NOT_FOUND_ERR when the window is missing

I don't really see why we should get the actor to perform the cleanUp here, so I'm enclined to go with the first option.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1674172 - [marionette] Move Marionette parent actor cleanUp to a static helper

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58ed22bd34dc
[marionette] Move Marionette parent actor cleanUp to a static helper r=marionette-reviewers,whimboo

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54dc92f44e8d
Fix bc failures on marionette/content/driver.js. r=jdescottes

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/58ed22bd34dc
https://hg.mozilla.org/mozilla-central/rev/54dc92f44e8d