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.

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.

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.

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!

Right. This is covered by my last patch. hopefully it should be in m-i this/next week.

Andrea, bug 1162088 has landed (thanks for you work) so we could review this one. Could you have a look a it please? Thanks!

(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?

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.

Comment on attachment 8616628 [details] [diff] [review] v2 https://treeherder.mozilla.org/#/jobs?repo=try&revision=1463067aea3b

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.

Carrying out r=baku https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7425dc7f9fc Let's see how try looks and land this patch.

(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.

I think this bug is a v1 one.

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