Closed Bug 1291387 Opened 8 years ago Closed 6 years ago

[geckoview] Platform testing for GeckoView: run reftest, Mochitest, web platform test suites

Categories

(GeckoView :: General, defect)

Product:
GeckoView
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: nalexander, Assigned: snorp)

References

Details

Attachments

(4 files)

Bug 1291387 - Make mochitest and reftest work against TestRunnerActivity
59 bytes, text/x-review-board-request
gbrown
: review+
jchen
: review+
Details
Bug 1291387 - Log GeckoThread state transitions
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1291387 - Handle errors when using EventDispatcher.sendRequestForResult in GeckoView JS modules
59 bytes, text/x-review-board-request
jchen
: review+
Details 
This ticket tracks running "Web Platform" tests against GeckoView.  In
theory this shouldn't be difficult; in reality, I think our existing
tests and harnesses make deep assumptions about the test environment
"being Firefox" that will make this difficult.  (B2G may have blazed a
lot of these trails already.)

This might look like a small GeckoView consuming Android application
which consumes test manifests, or communicates with the host like
SUTAgent, or ...

This is not required on day one: the existing Fennec tests give us
great confidence that the Web Platform implemented by Gecko is
functional on Android.

* Embedding testing: Android UI tests, Marionette tests

This ticket tracks testing the "GeckoView API".  This is new ground:
we built Robocop, which has not served the product well, and is now
significantly out of date, to do this for Fennec, but we'll need
something different for GeckoView.

My guess is that this looks like JUnit 4 + Espresso tests, or possibly
JUnit 4 + Marionette tests, running in a custom GeckoView consuming
Android application.
Gah, copy-paste fail.  The following is not part of this ticket, it is part of Bug 1291362:

> * Embedding testing: Android UI tests, Marionette tests
> 
> This ticket tracks testing the "GeckoView API".  This is new ground:
> we built Robocop, which has not served the product well, and is now
> significantly out of date, to do this for Fennec, but we'll need
> something different for GeckoView.
> 
> My guess is that this looks like JUnit 4 + Espresso tests, or possibly
> JUnit 4 + Marionette tests, running in a custom GeckoView consuming
> Android application.
Nick - Can you provide a quick update: Are we still interested in running mochitest/reftest/wpt in geckoview? Is the architecture worked out? Are we blocked on anything in particular?
Flags: needinfo?(nalexander)
(In reply to Geoff Brown [:gbrown] from comment #2)
> Nick - Can you provide a quick update: Are we still interested in running
> mochitest/reftest/wpt in geckoview? Is the architecture worked out? Are we
> blocked on anything in particular?

I can provide a quick update, but I'd best defer to snorp for the details.  It's my understanding that the GV project is still _very_ interested in running M/R/wpt in GeckoView.  They really want to make GV the thing that "implements the Web Platform", rather than how Fennec is the thing that implements it now.  That means Web-y tests should be exercising GV and not Fennec.  (Fennec-y functionality -- Sync, bookmark UI, menu interaction, etc -- still needs to be tested, of course.)

I don't think the architecture is really worked out, but I don't think it's all that complicated.  I think geckoview_example will grow harness functionality to make it really fast to run the various test suites.  That is, launch one Gecko, then connect multiple independent GeckoView instances to that Gecko and quickly run the various tests.  That'll be both faster and more robust than the current App-level manipulation we do.  And it makes sense for these special-purpose test harnesses to "help" the tests with special functionality, extra checking (leaks?), etc.

I think we're blocked on resources.  I'm certainly not involved with this project any more, and I'm not aware of anybody in snorp's group that is working on this.  Perhaps I am wrong.
Flags: needinfo?(nalexander) → needinfo?(snorp)
Yeah, we do want to run reftest/mochitest/etc in a GeckoView harness, but it's not yet clear how we get there. It probably won't be geckoview_example, but something else that is specialized for testing.
Flags: needinfo?(snorp)
The attached patch makes 'mach mochitest' and 'mach reftest' work against GeckoViewExample, but it should also work against any other GV-using app that has things set up appropriately.
Comment on attachment 8945567 [details]
Bug 1291387 - Make mochitest and reftest work against TestRunnerActivity

https://reviewboard.mozilla.org/r/215700/#review221484

I would like to see the 2 ANDROID_PACKAGE_NAME concerns addressed, but otherwise, this looks good. Thanks!

I thought the plan was to use something other than geckoview_example? Is this just path of least resistance? 

Should I look at using this solution in continuous integration?

::: layout/tools/reftest/mach_commands.py:141
(Diff revision 2)
>          args.ignoreWindowSize = True
>          args.printDeviceInfo = False
>  
>          from mozrunner.devices.android_device import grant_runtime_permissions, get_adb_path
> -        grant_runtime_permissions(self)
> +        if not args.app:
> +            args.app = 'org.mozilla.fennec'

args.app has already been set to ANDROID_PACKAGE_NAME just above (which is better!) so this shouldn't be needed.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
(Diff revision 2)
>              Log.d(LOGTAG, "onLoadUri=" + uri +
>                            " where=" + where);
> -            if (where != TargetWindow.NEW) {
> +            return where == TargetWindow.NEW;
> -                return false;
> -            }
> -            session.loadUri(uri);

No need for session.loadUri() here?

::: testing/mochitest/mach_commands.py:396
(Diff revision 2)
>  
>          if buildapp == 'android':
>              from mozrunner.devices.android_device import grant_runtime_permissions
> -            grant_runtime_permissions(self)
> +            app = kwargs.get('app')
> +            if not app:
> +                app = 'org.mozilla.fennec'

Should be able to use

app = self.substs["ANDROID_PACKAGE_NAME"]

here as well, to allow for org.mozilla.fennec_$USER, etc.
Attachment #8945567 - Flags: review?(gbrown) → review+
Comment on attachment 8945567 [details]
Bug 1291387 - Make mochitest and reftest work against TestRunnerActivity

https://reviewboard.mozilla.org/r/215700/#review221514

::: testing/mochitest/runtestsremote.py
(Diff revision 2)
>          kwargs.pop('marionette_args', None)
>  
>          ret, _ = self._automation.runApp(*args, **kwargs)
>          return ret, None
>  
> -

Removal of this blank line causes a lint error -- put it back!
Comment on attachment 8945567 [details]
Bug 1291387 - Make mochitest and reftest work against TestRunnerActivity

https://reviewboard.mozilla.org/r/215700/#review221808

r+ for mobile/ bits, but I'm thinking maybe we want to use our GeckoView instrumentation APK for this instead of geckoview_example.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:409
(Diff revision 2)
>          }
>  
>          // And go.
>          GeckoLoader.nativeRun(args, mCrashFileDescriptor, mIPCFileDescriptor);
>  
> +        Log.w(LOGTAG, "Gecko exited!");

Maybe we should just add logging to setState()

::: mobile/android/installer/package-manifest.in:298
(Diff revision 2)
>  @BINPATH@/components/EditorUtils.manifest
>  @BINPATH@/components/EditorUtils.js
>  
> -#ifndef MOZ_GECKOVIEW_JAR
>  @BINPATH@/components/extensions.manifest
> +@BINPATH@/components/addonManager.js

This is going to regress GeckoView startup time a lot :(
Attachment #8945567 - Flags: review?(nchen) → review+
Comment on attachment 8946301 [details]
Bug 1291387 - Log GeckoThread state transitions

https://reviewboard.mozilla.org/r/216268/#review222114
Attachment #8946301 - Flags: review?(nchen) → review+
I tried running our CI emulator mochitests and reftests against geckoview_example, using the first patch here. There are some good signs, but also a lot of failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02829e53f21f628683c25f20126058bf21c7a1ff

I see all reftests hitting the non-local network connections assertion, for mozilla.org access. In mochitests, there are lots of hangs and some crashes. I get very similar results when I run locally.
Comment on attachment 8945567 [details]
Bug 1291387 - Make mochitest and reftest work against TestRunnerActivity

https://reviewboard.mozilla.org/r/215700/#review224802


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mozbase/mozrunner/mozrunner/devices/android_device.py:228
(Diff revision 4)
>          # Installing every time is problematic because:
>          #  - it prevents testing against other builds (downloaded apk)
>          #  - installation may take a couple of minutes.
>          installed = emulator.dm.shellCheckOutput(['pm', 'list',
>                                                    'packages', 'org.mozilla.'])
> -        if 'fennec' not in installed and 'firefox' not in installed:
> +        if 'fennec' not in installed and 'firefox' not in installed and 'geckoview.test' not in installed:

Error: Line too long (106 > 99 characters) [flake8: E501]
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224806


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:125
(Diff revision 1)
> +      if (!sessionId) {
> +        session = null;
> +        return;
> +      }
> +
> +      gSessions.waitFor(sessionId, foundSession => {

Error: 'gsessions' is not defined. [eslint: no-undef]
See Also: → 1434423
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224896

Looks good, assuming it doesn't regress the target=_blank case.

::: mobile/android/chrome/geckoview/geckoview.js:29
(Diff revision 1)
>  // Instantiate a module by calling
>  //   add(<resource path>, <type name>)
>  // and remove by calling
>  //   remove(<type name>)
>  var ModuleManager = {
> -  init: function() {
> +  init: function(browser) {

aBrowser

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:1251
(Diff revision 1)
>           */
>          void onContextMenu(GeckoSession session, int screenX, int screenY,
>                             String uri, String elementSrc);
>      }
>  
> +    public interface Response<T> {

Please add documentation.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:55
(Diff revision 1)
>          final String intentArgs = getIntent().getStringExtra("args");
>  
>          if (BuildConfig.DEBUG) {
>              // In debug builds, we want to load JavaScript resources fresh with each build.
>              geckoArgs = "-purgecaches";
> +

Empty line.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:339
(Diff revision 1)
> -            }
> +        }
> -            session.loadUri(uri);
> -            return true;
> +
> +        @Override
> +        public void onNewSession(final GeckoSession session, final String uri, Response<GeckoSession> response) {
> +            final GeckoSession newSession = new GeckoSession(mGeckoSession.getSettings());
> +            newSession.openWindow(GeckoViewActivity.this);

We could add a clone method to take care of cloning and setting up a session based on an active session.

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:109
(Diff revision 1)
> +  handleNewSession(aUri, aOpener, aWhere, aFlags, aTriggeringPrincipal) {
> +    debug("handleNewSession: aUri=" + (aUri && aUri.spec) +
> +          " aWhere=" + aWhere +
> +          " aFlags=" + aFlags);
> +
> +    if (!this.isRegistered) {

I think shouldn't be getting this event, if not registered.

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:138
(Diff revision 1)
> +
> +    if (!session) {
> +      return null;
> +    }
> +
> +    return session ? session.browser : null;

Or session && session.browser, a pattern we use frequently in these modules.
Attachment #8949744 - Flags: review?(esawin) → review+
Comment on attachment 8949745 [details]
Bug 1291387 - Handle errors when using EventDispatcher.sendRequestForResult in GeckoView JS modules

https://reviewboard.mozilla.org/r/219062/#review224910
Attachment #8949745 - Flags: review?(droeh) → review+
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224898

There are some cases where two GeckoSession objects need to have the same ID (e.g. if you save the session to a Parcel and then read it back later, which we do to support saving/restoring GeckoView state). I think the easiest way to support that is to keep the ID as a property in GeckoSessionSettings, instead of as a final field in GeckoSession.

Also, I don't think we actually need `gSessions`. When we open a new nsWindow, we can set the name of the window to the ID string, and later we can use window watcher to get the window with the ID as its name.

::: mobile/android/chrome/geckoview/geckoview.js:90
(Diff revision 1)
>    ModuleManager.add("resource://gre/modules/GeckoViewTab.jsm",
>                      "GeckoViewTab");
>    ModuleManager.add("resource://gre/modules/GeckoViewRemoteDebugger.jsm",
>                      "GeckoViewRemoteDebugger");
>  
> -  // Move focus to the content window at the end of startup,
> +  browser.focus();

Please keep comment

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:73
(Diff revision 1)
>      private final EventDispatcher mEventDispatcher =
>          new EventDispatcher(mNativeQueue);
>  
>      private final TextInputController mTextInput = new TextInputController(this, mNativeQueue);
>  
> +    private final String mId = UUID.randomUUID().toString().replace("-", "");

Any reason for stripping the dashes?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:74
(Diff revision 1)
>          new EventDispatcher(mNativeQueue);
>  
>      private final TextInputController mTextInput = new TextInputController(this, mNativeQueue);
>  
> +    private final String mId = UUID.randomUUID().toString().replace("-", "");
> +    public String getId() { return mId; }

We should probably keep this package-private for now. The id strng is an implementation detail at this point.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:145
(Diff revision 1)
>                      callback.sendSuccess(result);
> +                } else if ("GeckoView:OnNewSession".equals(event)) {
> +                    final String uri = message.getString("uri");
> +                    listener.onNewSession(GeckoSession.this, uri,
> +                        new Response<GeckoSession>() {
> +                            public void respond(GeckoSession session) {

@Override

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:146
(Diff revision 1)
> +                } else if ("GeckoView:OnNewSession".equals(event)) {
> +                    final String uri = message.getString("uri");
> +                    listener.onNewSession(GeckoSession.this, uri,
> +                        new Response<GeckoSession>() {
> +                            public void respond(GeckoSession session) {
> +                                callback.sendSuccess(session.getId());

Session can be null here?

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:339
(Diff revision 1)
> -            }
> +        }
> -            session.loadUri(uri);
> -            return true;
> +
> +        @Override
> +        public void onNewSession(final GeckoSession session, final String uri, Response<GeckoSession> response) {
> +            final GeckoSession newSession = new GeckoSession(mGeckoSession.getSettings());
> +            newSession.openWindow(GeckoViewActivity.this);

I don't think the new session will be displayed because it doesn't have a surface. Is that intended behavior for geckoview_example?

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:109
(Diff revision 1)
> +  handleNewSession(aUri, aOpener, aWhere, aFlags, aTriggeringPrincipal) {
> +    debug("handleNewSession: aUri=" + (aUri && aUri.spec) +
> +          " aWhere=" + aWhere +
> +          " aFlags=" + aFlags);
> +
> +    if (!this.isRegistered) {

This is racy. We should always dispatch the event (and always handle the event in Java).

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:125
(Diff revision 1)
> +      if (!sessionId) {
> +        session = null;
> +        return;
> +      }
> +
> +      gSessions.waitFor(sessionId, foundSession => {

This would work nicely with `sendRequestForResult` if `waitFor` returned a Promise.

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:134
(Diff revision 1)
> +    });
> +
> +    // Wait indefinitely for a session to be created
> +    Services.tm.spinEventLoopUntil(() => session !== undefined);
> +
> +    if (!session) {

Redundant

::: mobile/android/modules/geckoview/GeckoViewSessions.jsm:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["gSessions"];

The export should just be called `GeckoViewSessions`

::: mobile/android/modules/geckoview/GeckoViewSessions.jsm:55
(Diff revision 1)
> +
> +    this._callbacks = callbacks;
> +  }
> +
> +  remove(window) {
> +    const id = window.arguments[0].QueryInterface(Ci.nsIAndroidView).id;

`remove()` is never called?

::: widget/android/nsIAndroidBridge.idl:68
(Diff revision 1)
>  
>  [scriptable, uuid(60a78a94-6117-432f-9d49-304913a931c5)]
>  interface nsIAndroidView : nsIAndroidEventDispatcher
>  {
>    [implicit_jscontext] readonly attribute jsval settings;
> +  [implicit_jscontext] readonly attribute string id;

I think we should just expose the id through `settings`. This is a lot of extra work that we can get for free.
Attachment #8949744 - Flags: review?(nchen) → review-
Comment on attachment 8949745 [details]
Bug 1291387 - Handle errors when using EventDispatcher.sendRequestForResult in GeckoView JS modules

https://reviewboard.mozilla.org/r/219062/#review224920

::: mobile/android/chrome/geckoview/GeckoViewContent.js:20
(Diff revision 1)
>  XPCOMUtils.defineLazyGetter(this, "dump", () =>
>      ChromeUtils.import("resource://gre/modules/AndroidLog.jsm",
>                         {}).AndroidLog.d.bind(null, "ViewContent"));
>  
>  function debug(aMsg) {
> -  // dump(aMsg);
> +  // dump(aMsg + "\n");

This shouldn't be necessary? logcat should add a new line for you.

::: mobile/android/chrome/geckoview/GeckoViewContent.js:190
(Diff revision 1)
> +      case "DOMWindowClose":
> +        if (!aEvent.isTrusted) {
> +          return;
> +        }
> +
> +        aEvent.preventDefault();

Please make sure this doesn't prevent window for custom tab from being closed when we close the custom tab.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:1230
(Diff revision 1)
>  
>          /**
> +        * A page has requested to close
> +        * @param session The GeckoSession that initiated the callback.
> +        */
> +        void onCloseRequest(GeckoSession session);

I think I'm on the fence between `ContentListener` and `NavigationListener`.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:184
(Diff revision 1)
> +            Log.i(LOGTAG, "Content requesting focus");
> +        }
> +
> +        @Override
> +        public void onCloseRequest(final GeckoSession session) {
> +            session.closeWindow();

`if (session != mGeckoSession)`, I think
Attachment #8949745 - Flags: review?(nchen) → review+
Comment on attachment 8945567 [details]
Bug 1291387 - Make mochitest and reftest work against TestRunnerActivity

https://reviewboard.mozilla.org/r/215700/#review224926

::: testing/mozbase/mozrunner/mozrunner/devices/android_device.py:228
(Diff revision 4)
>          # Installing every time is problematic because:
>          #  - it prevents testing against other builds (downloaded apk)
>          #  - installation may take a couple of minutes.
>          installed = emulator.dm.shellCheckOutput(['pm', 'list',
>                                                    'packages', 'org.mozilla.'])
> -        if 'fennec' not in installed and 'firefox' not in installed:
> +        if 'fennec' not in installed and 'firefox' not in installed and 'geckoview.test' not in installed:

I'll be handling this issue in bug 1434423.
(In reply to Eugen Sawin [:esawin] from comment #21)
> 
> :::
> mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/
> GeckoViewActivity.java:339
> (Diff revision 1)
> > -            }
> > +        }
> > -            session.loadUri(uri);
> > -            return true;
> > +
> > +        @Override
> > +        public void onNewSession(final GeckoSession session, final String uri, Response<GeckoSession> response) {
> > +            final GeckoSession newSession = new GeckoSession(mGeckoSession.getSettings());
> > +            newSession.openWindow(GeckoViewActivity.this);
> 
> We could add a clone method to take care of cloning and setting up a session
> based on an active session.

Maybe just 'public GeckoSession(GeckoSession original)'?

> 
> ::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:109
> (Diff revision 1)
> > +  handleNewSession(aUri, aOpener, aWhere, aFlags, aTriggeringPrincipal) {
> > +    debug("handleNewSession: aUri=" + (aUri && aUri.spec) +
> > +          " aWhere=" + aWhere +
> > +          " aFlags=" + aFlags);
> > +
> > +    if (!this.isRegistered) {
> 
> I think shouldn't be getting this event, if not registered.

It comes from the nsIBrowserDOMWindow::createContentWindow() impl, which is active all the time regardless of whether or not this module is registered. It's the same as the handleLoadUri() case I copied this from.
Flags: needinfo?(esawin)
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224898

Yeah, I ran across this earlier today. I think having GeckoSession Parcelable is wrong, because that implies that it can be sent to another process via Binder, which is not possible.

You're right that we could find the window via window watcher, but as I explain below, the window.presetOpenerWindow() must be called before the browser is appended to the window. We need an immediate hook between creating the browser and appending it to the window, which is what gSessions helps to provide.

> Any reason for stripping the dashes?

Not really, just aesthetics.

> I don't think the new session will be displayed because it doesn't have a surface. Is that intended behavior for geckoview_example?

Yeah, it's just to make stuff not blow up. I actually added this to get the tests running, but moved that out into the test runner activity now. Can probably just return null here.

> This is racy. We should always dispatch the event (and always handle the event in Java).

Can you explain the race? AFAICT it would only race with registering a ContentListener, which is not something I think we should worry about. Generally those are going to be setup ahead of time before you even load a page.

Handling this every time in GeckoSession would still yield a 'null' response in GeckoNavigationModule.createContentWindow, which is exactly what this does, so I don't think I'm following on why that would be necessary.

> This would work nicely with `sendRequestForResult` if `waitFor` returned a Promise.

Initially I wrote it to return a Promise, but the 'then' callbacks are executed in a future iteration of the main loop, which is too late for window.presetOpenerWindow(), as we've already added the <browser> to the window by then. I agree the callback thing is kinda gross, though, so more suggestions are welcome.

> `remove()` is never called?

Whoops! Yeah, need to hook that up.

> I think we should just expose the id through `settings`. This is a lot of extra work that we can get for free.

But it's not a setting, it's part of GeckoSession. I don't think we should piggyback on settings just because it already happens to be piped through to JS.
> > I think we should just expose the id through `settings`. This is a lot of extra work that we can get for free.
> 
> But it's not a setting, it's part of GeckoSession. I don't think we should
> piggyback on settings just because it already happens to be piped through to
> JS.

It seems like we'll have more use cases for "internal configs" which need to be passed to JS (e.g., TCP port, old TP flag), we could refactor settings to a more general container and make settings one of its properties.

> Maybe just 'public GeckoSession(GeckoSession original)'?

Sounds reasonable.
Flags: needinfo?(esawin)
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review225240

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:127
(Diff revision 1)
> +        return;
> +      }
> +
> +      gSessions.waitFor(sessionId, foundSession => {
> +        session = foundSession;
> +        session.browser.presetOpenerWindow(aOpener);

Hmm the session may not be a new session here. For an existing session, we can't call `presetOpenerWindow` so we have to call `setOpenerWindow`?
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224898

I don't think Parcelable by itself necessarily implies multiprocess. We use parcelable GeckoSession in-process to support saving/restoring GeckoView states. Also, shipping GeckoSession across processes is not necessarily out of the question if GeckoSession is running in a third service process.

As for window watcher, we can use 'chrome-document-interactive' notification as the hook between creating the browser and appending it to the window? `gSessions` to me just feels like reinventing a whole window registry for the sole purpose of being able to call `presetOpenerWindow()`.

> Can you explain the race? AFAICT it would only race with registering a ContentListener, which is not something I think we should worry about. Generally those are going to be setup ahead of time before you even load a page.
> 
> Handling this every time in GeckoSession would still yield a 'null' response in GeckoNavigationModule.createContentWindow, which is exactly what this does, so I don't think I'm following on why that would be necessary.

It can race with both registering and unregistering a ContentListener, and I don't think we should assume when the user is going to register/unregister. On registering it probably wouldn't be too bad. On unregistering, we can crash if the listener is null when we receive the event on the Java thread, so maybe all we need is a null check.
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review225240

> Hmm the session may not be a new session here. For an existing session, we can't call `presetOpenerWindow` so we have to call `setOpenerWindow`?

I think it needs to be a new session. I'll double check, but I don't think you can actually change the opener here. We should have tests around this too once we're able to do that. In any case, the docs need to lay out the expectations here and then we should enforce them as much as possible. I'll take care of this in the next patch.
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224898

re: Parcelable, I guess that's a good point. We should throw if you try to send it to another process today, though. I still don't want to put the id into the settings, though. I'll add it to the parcelable data instead so it's correctly preserved.

I'll try to hook up the window watcher stuff, as I agree that avoiding a session "manager" on the JS side would be preferable.

> It can race with both registering and unregistering a ContentListener, and I don't think we should assume when the user is going to register/unregister. On registering it probably wouldn't be too bad. On unregistering, we can crash if the listener is null when we receive the event on the Java thread, so maybe all we need is a null check.

You have a good point there re: unregistering. Since we're expected to be running in the same thread as setContentListener(), you're right that we just need a null check. It looks like GeckoSessionHandler already has this, so we should be in good shape.
Comment on attachment 8949745 [details]
Bug 1291387 - Handle errors when using EventDispatcher.sendRequestForResult in GeckoView JS modules

https://reviewboard.mozilla.org/r/219062/#review224920

> This shouldn't be necessary? logcat should add a new line for you.

Yep, looks like you're right. Removed.

> Please make sure this doesn't prevent window for custom tab from being closed when we close the custom tab.

In that case we're closing the containing XUL window directly, so there's no problem.

> I think I'm on the fence between `ContentListener` and `NavigationListener`.

Yeah, I thought NavigationListener could work too. No strong opinion on it though.

> `if (session != mGeckoSession)`, I think

This isn't supposed to happen, as content can only close windows that were opened with window.open(), however it seems like a good guard anyways.
Comment on attachment 8945567 [details]
Bug 1291387 - Make mochitest and reftest work against TestRunnerActivity

https://reviewboard.mozilla.org/r/215700/#review225764


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mozbase/mozrunner/mozrunner/devices/android_device.py:228
(Diff revision 5)
>          # Installing every time is problematic because:
>          #  - it prevents testing against other builds (downloaded apk)
>          #  - installation may take a couple of minutes.
>          installed = emulator.dm.shellCheckOutput(['pm', 'list',
>                                                    'packages', 'org.mozilla.'])
> -        if 'fennec' not in installed and 'firefox' not in installed:
> +        if 'fennec' not in installed and 'firefox' not in installed and 'geckoview.test' not in installed:

Error: Line too long (106 > 99 characters) [flake8: E501]
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review225770


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:117
(Diff revision 2)
> +            aSubject.browser.presetOpenerWindow(opener);
> +            Services.obs.removeObserver(handler, "geckoview-window-created");
> +            resolve(aSubject);
> +          }
> +        }
> +      }

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review225806

Need something sensible for the existing-window case, but looks okay otherwise.

::: mobile/android/chrome/geckoview/geckoview.js:14
(Diff revision 2)
>  ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  ChromeUtils.defineModuleGetter(this, "EventDispatcher",
>    "resource://gre/modules/Messaging.jsm");
> +  ChromeUtils.defineModuleGetter(this, "Services",

Alignment

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:112
(Diff revision 2)
> +
> +    return new Promise(resolve => {
> +      const handler = {
> +        observe(aSubject, aTopic, aData) {
> +          if (aTopic === "geckoview-window-created" && aSubject.name === sessionId) {
> +            aSubject.browser.presetOpenerWindow(opener);

Were you able to verify what happens if we use an existing window as the new window? I think if the window already exists, window watcher will actually set the opener properly for us, but we have to make sure we don't call `presetOpenerWindow` in that case, and skip waiting for "geckoview-window-created" altogether because we'll never get that notification.

If somehow we cannot use an existing window as the new window, we have to check for that and do something sensible like throw an exception.
Attachment #8949744 - Flags: review?(nchen) → review+
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224898

> Whoops! Yeah, need to hook that up.

I've eliminated GeckoViewSessions so this is moot now.
Comment on attachment 8949744 [details]
Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener registered

https://reviewboard.mozilla.org/r/219060/#review224898

> You have a good point there re: unregistering. Since we're expected to be running in the same thread as setContentListener(), you're right that we just need a null check. It looks like GeckoSessionHandler already has this, so we should be in good shape.

Actually, just realized we can't rely on the null check in GeckoSessionHandler, because it doesn't know to respond to the event. We'll end up indefinitely waiting for a response in GeckoViewNavigation.jsm.
(In reply to Jim Chen [:jchen] [:darchons] from comment #42)
> Comment on attachment 8949744 [details]
> Bug 1291387 - Reply with an error in GeckoSessionHandler when no listener
> registered
> 
> https://reviewboard.mozilla.org/r/219060/#review224898
> 
> > You have a good point there re: unregistering. Since we're expected to be running in the same thread as setContentListener(), you're right that we just need a null check. It looks like GeckoSessionHandler already has this, so we should be in good shape.
> 
> Actually, just realized we can't rely on the null check in
> GeckoSessionHandler, because it doesn't know to respond to the event. We'll
> end up indefinitely waiting for a response in GeckoViewNavigation.jsm.

I've moved this patch to bug 1432485 and addressed this concern in the latest revision there.
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8075cf4a68
Log GeckoThread state transitions r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/137940af4924
Reply with an error in GeckoSessionHandler when no listener registered r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd590aea9d04
Handle errors when using EventDispatcher.sendRequestForResult in GeckoView JS modules r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba294856f634
Make mochitest and reftest work against TestRunnerActivity r=gbrown,jchen
Assignee: nobody → snorp
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: