Closed Bug 1168226 Opened 7 years ago Closed 7 years ago

ServiceWorkerRegistrar only use the scope when registering a service worker

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
NGA S2 (12Jun)
Tracking Status
firefox41 --- fixed

People

(Reporter: jaoo, Assigned: jaoo)

References

Details

(Whiteboard: [s3])

Attachments

(1 file, 3 obsolete files)

STR required b2g:

1. In the browser app, opens appA.
2. appA registers a SW.
3. about:sw in b2g shows up SW.
3. Install AppA.
4. Open appA directly, appA registers the same SW again.
5. about:sw in b2g only shows up the SW registered at step #4.

Expected:

Both the SWs registered at step #2 and #4 should be listed in about:sw.
Assignee: nobody → jaoo
Status: NEW → ASSIGNED
At https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegistrar.cpp#138 the ServiceWorkerRegistrar obj registers the new service worker (SW). If the scope of the SW being registered is the same that an existing one it updates the data. Instead of comparing only the scope the logic should use the appId or something else so that way about:sw will list the two service workers (see comment 0). I'll update the title of the bug to reflect the issue here.
Status: ASSIGNED → NEW
Summary: about:servicesworkers does not show the correct service worker list in B2G → ServiceWorkerRegistrar only use the scope when registering a service worker
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
Rough patch that uses appId + IsInBrowser properties when ServiceWorkerRegistrar registers a service worker. Still need to deal with unregister but the patch shows the issue here.
Blocks: 1169249
Comment on attachment 8611430 [details] [diff] [review]
WIP

Andrea, it seems you took care of the unregister part in the latest version of the patch from bug 1162088 so there is no need to take care of it here. Could you provide some feedback here please? Thanks!
Attachment #8611430 - Flags: feedback?(amarchesini)
Right. This is covered by my last patch. hopefully it should be in m-i this/next week.
Whiteboard: [s3]
Target Milestone: --- → NGA S2 (12Jun)
Attached patch v1 (obsolete) — Splinter Review
Andrea, bug 1162088 has landed (thanks for you work) so we could review this one. Could you have a look a it please? Thanks!
Attachment #8611430 - Attachment is obsolete: true
Attachment #8611430 - Flags: feedback?(amarchesini)
Attachment #8615863 - Flags: feedback?(amarchesini)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> Created attachment 8615863 [details] [diff] [review]
> v1
> 
> Andrea, bug 1162088 has landed (thanks for you work) so we could review this
> one. Could you have a look a it please? Thanks!

I don't pretend to add the changes in b2g.js file, so ignore them please. I added some changed for testing this in b2g.
Comment on attachment 8615863 [details] [diff] [review]
v1

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

::: b2g/app/b2g.js
@@ +1141,1 @@
>  

This is not related, right?

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +148,5 @@
>    {
>      MonitorAutoLock lock(mMonitor);
>      MOZ_ASSERT(mDataLoaded);
>  
> +    const mozilla::ipc::PrincipalInfo& aInfo = aData.principal();

aInfo is a bad name. Use a better name.

@@ +154,4 @@
>      bool found = false;
>      for (uint32_t i = 0, len = mData.Length(); i < len; ++i) {
>        if (mData[i].scope() == aData.scope()) {
> +        const mozilla::ipc::PrincipalInfo& mInfo = mData[i].principal();

same for mInfo.

@@ +156,5 @@
>        if (mData[i].scope() == aData.scope()) {
> +        const mozilla::ipc::PrincipalInfo& mInfo = mData[i].principal();
> +
> +        if ((mInfo.type() == aInfo.type()) &&
> +            (mInfo.type() == mozilla::ipc::PrincipalInfo::TContentPrincipalInfo)) {

We support only this kind of PrincipalInfo. I guess you can do:

MOZ_ASSERT(mInfo.type() == mozilla::ipc::PrincipalInfo::TContentPrincipalInfo);

@@ +160,5 @@
> +            (mInfo.type() == mozilla::ipc::PrincipalInfo::TContentPrincipalInfo)) {
> +          const mozilla::ipc::ContentPrincipalInfo& mCInfo =
> +            mInfo.get_ContentPrincipalInfo();
> +          const mozilla::ipc::ContentPrincipalInfo& aCInfo =
> +            aInfo.get_ContentPrincipalInfo();

better names.

@@ +163,5 @@
> +          const mozilla::ipc::ContentPrincipalInfo& aCInfo =
> +            aInfo.get_ContentPrincipalInfo();
> +
> +          if ((mCInfo.appId() == aCInfo.appId()) &&
> +              (mCInfo.isInBrowserElement() == aCInfo.isInBrowserElement())) {

Check if PrincipalInfo has OriginAttributes. We are migrating from appId/BrowserElement to OriginAttributes everywhere.

@@ +171,5 @@
> +          }
> +
> +        } else {
> +          mData[i] = aData;
> +          found = true;

mmm always found?
Attachment #8615863 - Flags: feedback?(amarchesini) → feedback-
Attached patch v2 (obsolete) — Splinter Review
It doesn't use OriginAttributes as the == operator is implemented for mozilla::ipc::PrincipalInfo::TContentPrincipalInfo. Please, let me know if current patch would be ok or you really want me to use the OriginAttributes property.
Attachment #8615863 - Attachment is obsolete: true
Attachment #8616628 - Flags: feedback?(amarchesini)
Attachment #8616628 - Flags: feedback?(amarchesini) → feedback+
Comment on attachment 8616628 [details] [diff] [review]
v2

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

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +157,5 @@
> +        const mozilla::ipc::PrincipalInfo& existingPrincipalInfo =
> +          mData[i].principal();
> +
> +        MOZ_ASSERT(newPrincipalInfo.type() ==
> +                   mozilla::ipc::PrincipalInfo::TContentPrincipalInfo)

move it to line 153.

@@ +160,5 @@
> +        MOZ_ASSERT(newPrincipalInfo.type() ==
> +                   mozilla::ipc::PrincipalInfo::TContentPrincipalInfo)
> +
> +        const mozilla::ipc::ContentPrincipalInfo& newContentPrincipalInfo =
> +          newPrincipalInfo.get_ContentPrincipalInfo();

this one too.
Attachment #8616628 - Flags: review?(amarchesini) → review+
Attached patch v3Splinter Review
Carrying out r=baku

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

Let's see how try looks and land this patch.
Attachment #8616628 - Attachment is obsolete: true
Blocks: ServiceWorkers-B2G
No longer blocks: ServiceWorkers-v1
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #11)
> Created attachment 8620421 [details] [diff] [review]
> v3
> 
> Carrying out r=baku
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7425dc7f9fc
> 
> Let's see how try looks and land this patch.

Looking good, lets land this.
Keywords: checkin-needed
I think this bug is a v1 one.
See Also: → 1174083
Hi,

checked with today's (6/12) master build, both SWs appeared listed within about:sw as expected but a crash occurs once appA is installed, opened and both SWs are registered. It has been reported in Bug 1174083


Environmental variables:
Flame device
Build Id: 20150612080105
Gecko: f5cca86
Gaia: fcd90a0
Platform version: 41.0a1
You need to log in before you can comment on or make changes to this bug.