As mentioned in Bug 1163254, we will use originAttribute instead of appid and browserId. And also according to bug 1167100 comment 1 and comment 2, we will need some work in ServiceWorkerRegistrar.cpp

Hi Ben, In current flow, when the version of registrar is different, it will return error directly[1]. In this bug I will write origin attribute to registrar and change the version to 2. How do you think we should handle the version conflict ? just return error like what we implement now? or do some special handling to make version 1 registrar can still be read correctly? [1]https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegistrar.cpp#309

Long term we need to be migrating the registration file when we add a new version. Since we haven't shipped SW yet, though, I think we can just wipe the current file and start over. (Note, if we don't get the change in before the next branch merge we need a migration since we're shipping SW+push in 42.) Andrea, what do you think?

Remove write appid and browserId

bkelly, you are right. We have a version number of this file for this reason. But, I agree, we can ignore the migration since SW has not been shipped yet.

According to comment 3 and comment 5, this patch doesn't do registration file migration.

Comment on attachment 8646275 [details] [diff] [review] Patch V3 Review of attachment 8646275 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +321,5 @@ > if (NS_WARN_IF(!hasMoreLines)) { \ > return NS_ERROR_FAILURE; \ > } > > + nsAutoCString originAttributesSuffix; suffix is enough @@ +547,5 @@ > const mozilla::ipc::ContentPrincipalInfo& cInfo = > info.get_ContentPrincipalInfo(); > > + OriginAttributes attrs(cInfo.appId(), cInfo.isInBrowserElement()); > + nsAutoCString originAttributesSuffix; too

Address yoshi's comment

Comment on attachment 8647432 [details] [diff] [review] Patch v4 Review of attachment 8647432 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry, I don't know the registration code at all. I think Andrea would be a better reviewer here.

Comment on attachment 8647432 [details] [diff] [review] Patch v4 Review of attachment 8647432 [details] [diff] [review]: ----------------------------------------------------------------- You have to update the gtest too: dom/workers/test/gtest/TestReadWrite.cpp ::: dom/workers/ServiceWorkerRegistrar.h @@ +15,5 @@ > #include "nsString.h" > #include "nsTArray.h" > > #define SERVICEWORKERREGISTRAR_FILE "serviceworker.txt" > +#define SERVICEWORKERREGISTRAR_VERSION "2" don't change the version. Otherwise you have to support migration from 1 to 2.

(In reply to Andrea Marchesini (:baku) from comment #10) > don't change the version. Otherwise you have to support migration from 1 to > 2. Don't we need to wipe the old file somehow? How does that work without changing the version?

Hi Andrea, If we don't change the version, how do SW distinguish old version and new version ? Do you mean we don't handle this case and expect it will fail somewhere inside |ServiceWorkerRegistrar::ReadData|? I thought the plan about ignoring the migration is when the version is different, we just delete the old one[1]. [1]https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegistrar.cpp#251

> [1]https://dxr.mozilla.org/mozilla-central/source/dom/workers/ > ServiceWorkerRegistrar.cpp#251 Do we have SW enabled in non-nightly builds? If yes, then change it to version 2. Otherwise I don't see the point to increase the version.

Andrea, but even for nightly users... leaving the version unchanged causes the file to be corrupt, right? That seems bad... Also, we have shipped service workers for push in firefox 42.

OK. dlee, can you submit a new patch with version changed + gtest updates?

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ed7f4aba59