Closed Bug 963239 Opened 6 years ago Closed 6 years ago

Stop fetching b2g shell via getMostRecentWindow(navigator:browser)

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 10 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #8364602 - Flags: feedback?(fabrice)
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.
Attachment #8364602 - Flags: feedback?(fabrice) → feedback+
Duplicate of this bug: 962183
Since you're touching `sendCustomEvent()`, could you please make it return the event, so that senders can verify if `event.isDefaultPrevented()` after sending it?
Duplicate of this bug: 962572
Attached patch patch (obsolete) — Splinter Review
Rebased and fixed some issues discovered when testing on device.
Attachment #8364602 - Attachment is obsolete: true
Attached patch tests (obsolete) — Splinter Review
Add a mochitest against SystemApp.jsm
Depends on: 965257
Attached patch tests (obsolete) — Splinter Review
Attachment #8367322 - Attachment is obsolete: true
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
Attached patch patch with tests (obsolete) — Splinter Review
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #10)
Thanks for the report, I updated the patch.
Attachment #8367321 - Attachment is obsolete: true
Attachment #8367893 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Rebased patch, still waiting on bug 965257 before proceeding with this patch.
https://tbpl.mozilla.org/?tree=Try&rev=5670f35f5879
Attachment #8374129 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Just rebased.
https://tbpl.mozilla.org/?tree=Try&rev=4e12fa2c9fe1
Attachment #8384886 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Uses Cu.cloneInto instead of ObjectWrapper.
Attachment #8396300 - Attachment is obsolete: true
Assignee: nobody → poirot.alex
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.
Attachment #8396443 - Flags: review?(21)
Attachment #8396443 - Flags: feedback?(fabrice)
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 ? :)
Attachment #8396443 - Flags: review?(21)
Attachment #8396443 - Flags: feedback?(fabrice)
Attachment #8396443 - Flags: feedback+
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8396443 - Attachment is obsolete: true
Attachment #8400778 - Flags: review?(21)
(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.
(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 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 ?
Attachment #8400778 - Flags: review?(21) → review+
Attached patch patch (obsolete) — Splinter Review
Addressed last review, also fixed some indentation and unnecessary modification made to updateprompt and webappsupdater.

https://tbpl.mozilla.org/?tree=Try&rev=0209a7ea7064
Attachment #8400778 - Attachment is obsolete: true
Attached patch patchSplinter Review
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
Attachment #8401290 - Attachment is obsolete: true
Attachment #8401419 - Flags: review+
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.
Try is green!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c50c4788f313
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
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.
Flags: needinfo?(poirot.alex)
Let's discuss about that in bug 996528
Flags: needinfo?(poirot.alex)
No longer blocks: 1015034
Depends on: 1015034
Depends on: 1040932
You need to log in before you can comment on or make changes to this bug.