963239 - Stop fetching b2g shell via getMostRecentWindow(navigator:browser)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

In the Mulet, b2g's shell.html document isn't the top level chrome window anymore. Instead, it becomes a document loaded in a regular firefox tab.

So that, all code similar to this one are going to fail on the Mulet:
  let browser = Services.wm.getMostRecentWindow("navigator:browser");
  if (browser && browser.shell) {
    browser.shell.sendChromeEvent(event);

We should make it easy to support both environmenet and ease communicating with the system app when you are in a platform code.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch

Implements "SystemApp.jsm" to abtract and ease interacting with the system app from platform code.

This jsm exposes only the core features, the platform code needs to interact with gaia via the system app: send and receive events.
It factors out some code related to pending events, duplicated between shell.js, UpdatesPrompt and WebappsUpdater.
It then makes other codepaths stronger by always have the pending event support.
It may also fix the random errors when Services.vm.getMostRecentWindow(navigator:browser).shell.contentBrowser was undefined!
All components trying to listen for content events seem to have all faced scenarios where
the system app iframe wasn't created yet, while they were already trying to listen for events.
In this little helper we can also have "pending listeners", to prevent any such startup race.

fabrice, before I start testing this patch extensively, can you confirm that this sounds good to you?

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8364602 [details] [diff] [review]
patch

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

::: b2g/components/ActivitiesGlue.js
@@ +13,5 @@
>  
> +XPCOMUtils.defineLazyGetter(this, 'SystemApp', function() {
> +  Cu.import('resource://gre/modules/SystemApp.jsm');
> +  return SystemApp;
> +});

There's XPCOMUtils.defineLazyModuleGetter that does that for you already.

Comment 4

[Jan Keromnes [:janx]]

Since you're touching `sendCustomEvent()`, could you please make it return the event, so that senders can verify if `event.isDefaultPrevented()` after sending it?

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch

Rebased and fixed some issues discovered when testing on device.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: tests

Add a mochitest against SystemApp.jsm

Comment 8

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=a8bb0309a92d

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: tests

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hi @Alexandre, previously I add some test cases that use getMostRecentWindow(navigator:browser) [1]. You might need to modify it as well. :)

[1] http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/test/mochitest/RecordingStatusChromeScript.js#20

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: patch with tests

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #10)
Thanks for the report, I updated the patch.

Comment 12

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=95f4b11dfbd3

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: patch

Rebased patch, still waiting on bug 965257 before proceeding with this patch.
https://tbpl.mozilla.org/?tree=Try&rev=5670f35f5879

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: patch

Just rebased.
https://tbpl.mozilla.org/?tree=Try&rev=4e12fa2c9fe1

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: patch

Uses Cu.cloneInto instead of ObjectWrapper.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8396443 [details] [diff] [review]
patch

This patch touches various code, do not hesitate to flag anyone else for review.

It basically exposes a new JSM, SystemApp which allows to easily
listen for events on the system app as well as dispatch events on it.
It has be to be used everywhere in platform code in order to ensure b2g to work on the mulet.

I'll post on dev-b2g about this once reviewed.

Fabrice, I'm asking vivien to review this patch as I already have other patches pending you for review. So I'm trying to balance reviews here, but do not hesitate to review this one as well.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8396443 [details] [diff] [review]
patch

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

It's a nice clean up! I would like to enforce a bit the policy about how to use mozChromeEvent/mozContentEvent and abstract that so people will not mess up with Gaia and creates tons of new events however.

In the future it would also be nice if the |id| check for a message that is supposed to be a return value of a chrome event could be done into this helper. That would make the code more robust.

::: b2g/chrome/content/shell.js
@@ +30,5 @@
>  
>  Cu.import('resource://gre/modules/DownloadsAPI.jsm');
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SystemApp",
> +                                  "resource://gre/modules/SystemApp.jsm");

Can we give it a better name, something like SystemAppProxy.jsm ?

@@ +672,5 @@
>  #endif
>  
>        Services.obs.notifyObservers(null, "browser-ui-startup-complete", "");
>  
> +      SystemApp.setIsLoaded();

SystemAppProxy.setIsReady();

::: b2g/chrome/content/test/mochitest/RecordingStatusChromeScript.js
@@ +2,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  const { Services } = Cu.import('resource://gre/modules/Services.jsm');
> +const { SystemApp } = Cu.import("resource://gre/modules/SystemApp.jsm");

s/"/'

::: b2g/components/FxAccountsMgmtService.jsm
@@ +36,2 @@
>    _onFulfill: function(aMsgId, aData) {
> +    SystemApp.sendCustomEvent("mozFxAccountsChromeEvent", {

Those should not be allowed to send a customEvent. The dirty mozChromeEvent/mozContentEvent hack is not intended to start to have tons of custom events.

Same comments for the others.

Sorry to put that on your plate but since you're refactoring the mechanism to discuss from Gecko to the System App it seems a good time to enforce some practices.

::: b2g/components/FxAccountsUIGlue.js
@@ +29,3 @@
>      let id = uuidgen.generateUUID().toString();
>  
> +    SystemApp.addEventListener("mozFxAccountsRPContentEvent",

Same comment than the other FXAccountMgmtService file.

::: b2g/components/SystemApp.jsm
@@ +21,5 @@
> +  // either the system app one, or a top level xul window one,
> +  // if the system app isn't loaded yet.
> +  get navigator() {
> +    return this._frame ? this._frame.contentWindow.navigator
> +                       : Services.wm.getMostRecentWindow('navigator:browser').navigator;

In which case do we want to return the system app navigator object instead of the top xul level window ? The system app can miss some permissions (ok that's unlikely).

@@ +22,5 @@
> +  // if the system app isn't loaded yet.
> +  get navigator() {
> +    return this._frame ? this._frame.contentWindow.navigator
> +                       : Services.wm.getMostRecentWindow('navigator:browser').navigator;
> +  },

More generally I don't think this method should be part of this module. People should use an XPCOM if they want to access the mozApps database or the ICC status. That's fine for the purpose of this patch but can you open a followup to remove the navigator calls inside the xpcom components ?

@@ +38,5 @@
> +  },
> +
> +  // To call when it is ready to receive events
> +  setIsLoaded: function () {
> +    this._isLoaded = true;

Maybe you want to throw if someone call setIsReady when the module is already ready ?

@@ +51,5 @@
> +  /*
> +   * Common way to send an event to the system app.
> +   *
> +   * // In gecko code:
> +   *   SystemApp.sendCustomEvent('foo', { data: 'bar' });

Seems like there is a mix of ' and " in this file :P

@@ +57,5 @@
> +   *   window.addEventListener('foo', function (event) {
> +   *     event.details == 'bar'
> +   *   });
> +   */
> +  sendCustomEvent: function systemApp_sendCustomEvent(type, details) {

People should not really be able to specify a custom event name. Specific types should be passed into the |type| field of the event detail.

Can you enforce the 'mozChromeEvent' name here and remove the type parameters. Then you can also remove the sendCustomEvent method and rename sendCustomEvent method to dispatchEvent.

@@ +65,5 @@
> +    // queue events until someone calls setIsLoaded
> +    if (!this._isLoaded || !content) {
> +      this._pendingChromeEvents.push([type, details]);
> +      return null;
> +    }

nit: add a line break after this block since this block will return. That makes it easier to see.

@@ +77,5 @@
> +    } else {
> +      payload = details ? Cu.cloneInto(details, content) : {};
> +    }
> +
> +    event.initCustomEvent(type, true, true, payload);

I don't think any of those events are really cancelable. Let turn the third parameter to false.

@@ +92,5 @@
> +  // Listen for dom events on the system app
> +  addEventListener: function systemApp_addEventListener() {
> +    let content = this._frame ? this._frame.contentWindow : null;
> +    if (!content) {
> +      Cu.reportError("Trying to add a system app event listener while the app is still loading");

Is it really an error ? Not sure this message won't just be pain in the ass.

@@ +97,5 @@
> +      this._pendingListeners.push(arguments);
> +      return false;
> +    }
> +
> +    content.addEventListener.apply(content, arguments);

Can we abstract the mozContentEvent name here ? In theory people should not listen anything else than mozContentEvent and I would be happy if this is enforced by the helper.

It also seems like the fx accounts code does not do that... That's bad. The type of events likely needs to lived into a |type| field of event.detail.

@@ +105,5 @@
> +  removeEventListener: function systemApp_removeEventListener() {
> +    let content = this._frame ? this._frame.contentWindow : null;
> +    if (content) {
> +      content.removeEventListener.apply(content, arguments);
> +    }

This function does not seems to remove something from the pendingListeners. So if someone had a listener then decide to remove it or some reasons it will still be in the pendingListeners list.

We can likely abstract the 'mozContentEvent' type parameters as well.

::: b2g/components/test/mochitest/systemapp_helper.js
@@ +65,5 @@
> +    });
> +  },
> +
> +  function earlyEvents() {
> +    // Immediatly try to send events

s/Immediatly/Immediately

::: b2g/components/test/mochitest/test_systemapp.html
@@ +4,5 @@
> +https://bugzilla.mozilla.org/show_bug.cgi?id=963239
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Permission Prompt Test</title>

Permission prompt test ? :)

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)

> It's a nice clean up! I would like to enforce a bit the policy about how to
> use mozChromeEvent/mozContentEvent and abstract that so people will not mess
> up with Gaia and creates tons of new events however.

I guess we'll need to discuss that outside of this bug, but I disagree that using just mozChromeEvent is good. It already caused us performance issues with app launch time because we had > 20 listeners for this event and the one dealing with apps launching was one of the last ones. That meant ~30ms lost for nothing.

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: patch

Addressed most comments.
I avoided modifying the actual implementation of each b2g components.
Modifying the way these components communicate with the system app
requires a lot of work (gecko and gaia patches, that can potentially break each other).

For example, ContentPermissionPrompt.js with its sendToBrowserWindow helper function,
doesn't listen for any explicit mozContentEvent with one precise type,
but rather filter by id...

So the suggestion to listen for one precise type of mozContentEvent
doesn't really work as-is with this component.

Similar kind of complexity also happens with fx account code.

Whereas I can understand such cleanup, it really has nothing to do with the Mulet,
and such refactoring should be made by each component owner,
which know exatly how to properly test all this code.

Also, instead of forcing a refactoring by only exposing a clean API,
we should first expose what we think is a better way to communicate with the system app,
and then refactor each component one by one.

Comment 20

[Alexandre Poirot [:ochameau]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)
> ::: b2g/chrome/content/shell.js
> @@ +30,5 @@
> >  
> >  Cu.import('resource://gre/modules/DownloadsAPI.jsm');
> >  
> > +XPCOMUtils.defineLazyModuleGetter(this, "SystemApp",
> > +                                  "resource://gre/modules/SystemApp.jsm");
> 
> Can we give it a better name, something like SystemAppProxy.jsm ?

Done

> ::: b2g/components/FxAccountsMgmtService.jsm
> @@ +36,2 @@
> >    _onFulfill: function(aMsgId, aData) {
> > +    SystemApp.sendCustomEvent("mozFxAccountsChromeEvent", {
> 
> Those should not be allowed to send a customEvent. The dirty
> mozChromeEvent/mozContentEvent hack is not intended to start to have tons of
> custom events.
> 
> Same comments for the others.
> 
> Sorry to put that on your plate but since you're refactoring the mechanism
> to discuss from Gecko to the System App it seems a good time to enforce some
> practices.
> 

Renamed to SystemApp._sendCustomEvent, to at least say it better to use sendChromeEvent.

> ::: b2g/components/SystemApp.jsm
> @@ +21,5 @@
> > +  // either the system app one, or a top level xul window one,
> > +  // if the system app isn't loaded yet.
> > +  get navigator() {
> > +    return this._frame ? this._frame.contentWindow.navigator
> > +                       : Services.wm.getMostRecentWindow('navigator:browser').navigator;
> 
> In which case do we want to return the system app navigator object instead
> of the top xul level window ? The system app can miss some permissions (ok
> that's unlikely).

It's not really important, I switched back all call site to use navigator:browser as before.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] NOT READING BUGMAIL from comment #18)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)
> 
> > It's a nice clean up! I would like to enforce a bit the policy about how to
> > use mozChromeEvent/mozContentEvent and abstract that so people will not mess
> > up with Gaia and creates tons of new events however.
> 
> I guess we'll need to discuss that outside of this bug, but I disagree that
> using just mozChromeEvent is good. It already caused us performance issues
> with app launch time because we had > 20 listeners for this event and the
> one dealing with apps launching was one of the last ones. That meant ~30ms
> lost for nothing.

That should be fixeable directly in the system app if we apply the same rule. 
We could have one mozChromeListener in the system app that redispatch those events as custom events with different names.

But instead of having multiple places that are dispatching tons of different events we have one place that we control and may be able to clean at some point.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8400778 [details] [diff] [review]
patch

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

r+ with SystemAppProxy.removeEventListener removing events from pendings.

::: b2g/chrome/content/payment.js
@@ +239,5 @@
>      // In order to avoid race conditions, we wait for the UI to notify that
>      // it has successfully closed the payment flow and has recovered the
>      // caller app, before notifying the parent process to fire the success
>      // or error event over the DOMRequest.
> +    SystemAppProxy.addEventListener(kClosePaymentFlowEvent,

mozContentEvent ?

::: b2g/components/SystemAppProxy.jsm
@@ +12,5 @@
> +this.EXPORTED_SYMBOLS = ['SystemAppProxy'];
> +
> +let SystemAppProxy = {
> +  _frame: null,
> +  _isLoaded: false,

s/_isLoaded/_isReady/

@@ +13,5 @@
> +
> +let SystemAppProxy = {
> +  _frame: null,
> +  _isLoaded: false,
> +  _pendingChromeEvents: [],

s/_pendingChromeEvents/_pendingEvents/

@@ +100,5 @@
> +  removeEventListener: function systemApp_removeEventListener() {
> +    let content = this._frame ? this._frame.contentWindow : null;
> +    if (content) {
> +      content.removeEventListener.apply(content, arguments);
> +    }

You want to iterate over the pending events in the case where there is no content.

::: b2g/components/test/mochitest/systemapp_helper.js
@@ +1,2 @@
> +const Cu = Components.utils;
> +

Can you add a test to check that removeEventListener removes events from the pending lists as well ?

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: patch

Addressed last review, also fixed some indentation and unnecessary modification made to updateprompt and webappsupdater.

https://tbpl.mozilla.org/?tree=Try&rev=0209a7ea7064

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: patch

Fixed two failing tests.
One typo _pendingListener instead of _pendingListeners,
and I had to modify fxaccount xpcshell test.

https://tbpl.mozilla.org/?tree=Try&rev=3283878575e1

Comment 25

[Alexandre Poirot [:ochameau]]

Manually tested:
  Activities, by changing the wallpaper
  ContentPermissions, by getting the prompt in Here maps and correct location
  FxAccount, with Test FxA app
  SignIn, with UI test
  Webapps, Got two hosted and one packaged app being updated with correct notification

Poorly tested:
  Payment, Don't know how to test, blocked on pin code question in the in-app payment test app.
  Update, Don't know how to test.

Comment 26

[Alexandre Poirot [:ochameau]]

Try is green!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c50c4788f313

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Yuan Xulei [:yxl] (Leave from May 18)]

Gaia keyboard needs sendChromeEvent when running in Firefox Nightly. This bug broke that. Could you make SystemApp.jsm available on Firefox Nightly as well?

See bug 996528 for details.

Comment 30

[Alexandre Poirot [:ochameau]]

Let's discuss about that in bug 996528