Closed Bug 801573 Opened 7 years ago Closed 7 years ago

[Web Activities] Need to notify SystemMessageInternal when the apps' registration restarts, to avoid sending system messages to deprecated pages.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

Attachments

(1 file, 2 obsolete files)

The apps' registration can happen at any time like updating apps after booting up. SystemMessageInternal needs to know when the activities starts to do that at run-time, so that it won't have chances to send the system messages to the wrong pages.
Assignee: nobody → clian
Blocks: 715814, 795209
Attached patch Patch (obsolete) — Splinter Review
Hi Vivien and Fabrice,

Not sure who is the expert of this part, so temporally assign this to you guys. Could either of you take a look on this patch? Thanks a lot!

Summary:

  1. We need to send an observer message "webapps-registry-start" to SystemMessageInternal, so that the SystemMessageInternal will buffer all the system messages until the apps' registration is redone (i.e. until receiving "webapps-registry-ready").

  2. I don't see any reason why we need a flag |this.allActivitiesSent|. Tentatively clean it up. Please correct me if I'm wrong.

  3. Not every part needs |this._saveApps()| so I didn't put it in the |this.notifyAppsRegistryReady()|.
Attachment #671382 - Flags: review?(fabrice)
Attachment #671382 - Flags: review?(21)
Comment on attachment 671382 [details] [diff] [review]
Patch

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

Leaving the review to fabrice since it is the expert here :)
Attachment #671382 - Flags: review?(21)
(In reply to Gene Lian [:gene] from comment #1)
> Created attachment 671382 [details] [diff] [review]
> Patch
> 
> Hi Vivien and Fabrice,
> 
> Not sure who is the expert of this part, so temporally assign this to you
> guys. Could either of you take a look on this patch? Thanks a lot!
> 
> Summary:
> 
>   1. We need to send an observer message "webapps-registry-start" to
> SystemMessageInternal, so that the SystemMessageInternal will buffer all the
> system messages until the apps' registration is redone (i.e. until receiving
> "webapps-registry-ready").

Hm, ok. I understand why you need that, but we also have another listener for webapps-registry-ready (http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#501) and we need to make sure that calling it several times is ok.

>   2. I don't see any reason why we need a flag |this.allActivitiesSent|.
> Tentatively clean it up. Please correct me if I'm wrong.

yep, you can't change that. The activities registration is async and we need that flag to prevent race conditions.

>   3. Not every part needs |this._saveApps()| so I didn't put it in the
> |this.notifyAppsRegistryReady()|.

I need to take a closer look for this one, but at first sight I would rather keep it in notifyAppsRegistryReady(). This is not too costly.
Attachment #671382 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #3)
> >   1. We need to send an observer message "webapps-registry-start" to
> > SystemMessageInternal, so that the SystemMessageInternal will buffer all the
> > system messages until the apps' registration is redone (i.e. until receiving
> > "webapps-registry-ready").
> 
> Hm, ok. I understand why you need that, but we also have another listener
> for webapps-registry-ready
> (http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.
> js#501) and we need to make sure that calling it several times is ok.

Thanks for the reminder!

So far, Gaia only listens to the "webapps-registry-ready" event *one time* for initialization and removes the listener immediately [1], so I think there should be no harm if we fire multiple times. I think I can fire another "webapps-registry-start" event if Gaia really needs it in the future to know the time period for apps' registration.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/applications.js#L32

> 
> >   2. I don't see any reason why we need a flag |this.allActivitiesSent|.
> > Tentatively clean it up. Please correct me if I'm wrong.
> 
> yep, you can't change that. The activities registration is async and we need
> that flag to prevent race conditions.

All right. I understand it better :) I'll also add this flag in other places wherever we call |this._registerActivities()|.

> 
> >   3. Not every part needs |this._saveApps()| so I didn't put it in the
> > |this.notifyAppsRegistryReady()|.
> 
> I need to take a closer look for this one, but at first sight I would rather
> keep it in notifyAppsRegistryReady(). This is not too costly.

OK. I'll put |this._saveApps()| in |this.notifyAppsRegistryReady()|.
Attached patch Patch V2 (obsolete) — Splinter Review
Please see comment #4. Thanks Fabrice for the review! :)
Attachment #671382 - Attachment is obsolete: true
Attachment #671713 - Flags: review?(fabrice)
Comment on attachment 671713 [details] [diff] [review]
Patch V2

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

r=me with comment addressed, and I'd also like to see a try run of this patch.

::: dom/apps/src/Webapps.jsm
@@ +1000,4 @@
>  #ifdef MOZ_SYS_MSG
> +      this.activitiesToRegister = 0;
> +      this.activitiesRegistered = 0;
> +      this.allActivitiesSent = false;

So, we now have three places where we do these initializations. That's very error prone, please put them in a common helper function.
Attached patch Patch V2.1Splinter Review
Addressing comment 6. Thanks Fabrice for the review.
Attachment #671713 - Attachment is obsolete: true
Attachment #671713 - Flags: review?(fabrice)
Attachment #672157 - Flags: review+
(In reply to Gene Lian [:gene] from comment #8)
> Try server: https://tbpl.mozilla.org/?tree=Try&rev=99276143e5e8 (all green!
> magic!)

Sorry, not everything is green but should still be fine. ;)
https://hg.mozilla.org/mozilla-central/rev/357778ffa801
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(In reply to Ryan VanderMeulen from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/357778ffa801
> 
> Should this have a test?

No, thanks!
Flags: in-testsuite? → in-testsuite-
Please specify this by bb+ because it needs to be checked in into Aurora.
blocking-basecamp: --- → ?
Blocks: 799161
Blocks: 796293
Blocks blockers.
blocking-basecamp: ? → +
(In reply to Gene Lian [:gene] from comment #13)
> Please specify this by bb+ because it needs to be checked in into Aurora.

Just to clarify: you don't need basecamp-blocking+ to push in m-a. You can also get approval without this.
Thanks Ryan, Fabrice, Chris and Mounir for your drive-by supports and comments. Glad to know everything was perfectly done after the weekend. :)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.