Closed Bug 1674172 Opened 4 years ago Closed 4 years ago

getMarionetteCommandsActorProxy doesn't check for a valid browsing context

Categories

(Remote Protocol :: Marionette, defect, P3)

Default
defect

Tracking

(Fission Milestone:M7, firefox84 fixed)

RESOLVED FIXED
84 Branch
Fission Milestone M7
Tracking Status
firefox84 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

Details

(Whiteboard: [marionette-fission-mvp])

Attachments

(1 file)

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?

Flags: needinfo?(jdescottes)

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

Fission Milestone: --- → M7

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

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.

Flags: needinfo?(jdescottes)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/58ed22bd34dc [marionette] Move Marionette parent actor cleanUp to a static helper r=marionette-reviewers,whimboo
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/54dc92f44e8d Fix bc failures on marionette/content/driver.js. r=jdescottes
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: