1155153 - [e10s] about:serviceworkers should work in e10s mode

Status: RESOLVED FIXED

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

Description

[Tim Taubert [:ttaubert] (inactive)]

about:serviceworkers was implemented without e10s support. We need to make it work with e10s and load correctly in a child process.

Comment 1

[(no longer active)]

(In reply to Tim Taubert [:ttaubert] from comment #0)
> about:serviceworkers was implemented without e10s support. We need to make
> it work with e10s and load correctly in a child process.

Why?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Sorry I didn't respond earlier. There currently are a ton of pages that don't support e10s but this probably won't change anytime soon. I had some more time to think about this and talked to a few people, I think we will have to support seamless switching between remote and non-remote browsers for the time being if we ever want to ship e10s.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #1)
> (In reply to Tim Taubert [:ttaubert] from comment #0)
> > about:serviceworkers was implemented without e10s support. We need to make
> > it work with e10s and load correctly in a child process.
> 
> Why?

Because both the update and unregister buttons don't work. The page displays all the registration but it doesn't update them correctly. If I am not wrong this is because the page is updating the registration in the parent process. It only works when e10s mode is disabled. To make these button work (with e10s mode enabled) the page should tell the corresponding child process to update the registration.

Right now if the user tries to update a registration when e10s mode is enabled the following waring is seen:

[Parent 83643] WARNING: '!registration', file /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 2848

Eshan, am I wrong? Thanks!

Comment 4

[(no longer active)]

about:serviceworkers should load in the parent process if you have e10s disabled, those buttons do work for me.  How are you loading it in the child process?

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> about:serviceworkers should load in the parent process if you have e10s
> disabled, those buttons do work for me.  How are you loading it in the child
> process?

Those buttons don't work for me when e10s is enabled, do they work for you with e10s mode enabled? They should work always I guess no matter what value the e10s mode has. As I told you above if the user tries to update a registration when e10s mode is enabled the following waring is seen:

[Parent 83643] WARNING: '!registration', file /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 2848

Comment 6

[(no longer active)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #4)
> > about:serviceworkers should load in the parent process if you have e10s
> > disabled, those buttons do work for me.  How are you loading it in the child
> > process?
> 
> Those buttons don't work for me when e10s is enabled, do they work for you
> with e10s mode enabled?

Yes, they do, according to a quick test, at least.

> They should work always I guess no matter what value
> the e10s mode has.

The thing is, about:serviceworkers is not marked with URI_CAN_LOAD_IN_CHILD, so it should always be loaded in the parent process.

> As I told you above if the user tries to update a
> registration when e10s mode is enabled the following waring is seen:
> 
> [Parent 83643] WARNING: '!registration', file
> /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp,
> line 2848

What code does that point to?  (It seems like your tree doesn't match either mine or mozilla-central.)  Depending on where that code is this warning may be ignored or may be very serious!

Anyways, reopening and CCing baku as he will probably be the right person to debug and fix this.

Comment 7

[(no longer active)]

(Note that you still haven't described how you get about:sw to load in the child process... that should never happen!)

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> (Note that you still haven't described how you get about:sw to load in the
> child process... that should never happen!)

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #3)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #1)

> To make these button work (with
> e10s mode enabled) the page should tell the corresponding child process to
> update the registration.

I didn't mean I had a way to get the page to load in the child process. I was trying to say that I guess to make these button work (with e10s mode enabled) the page should tell the corresponding child process to update the registration. Sorry If I didn't explain myself clearly.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> > As I told you above if the user tries to update a
> > registration when e10s mode is enabled the following waring is seen:
> > 
> > [Parent 83643] WARNING: '!registration', file
> > /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp,
> > line 2848
> 
> What code does that point to?  (It seems like your tree doesn't match either
> mine or mozilla-central.)  Depending on where that code is this warning may
> be ignored or may be very serious!

I had a few patches applied. That warning comes from https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2818.

Comment 9

[(no longer active)]

Andrea, can you please investigate?  Thanks!

Comment 10

[Andrea Marchesini [:baku]]

:ehsan, initially about:sw was running only on the parent process. But then we tried (during the work-week in Madrid) to expose this page to b2g. Here we need to have it running in the child process in order to have it accessible from the browser app.

I don't see why we cannot have about:sw in the child process. What is missing is the propagation of the unregistration/registration information. Tell me if I'm wrong but:

1. ChildProcess wants to unregister a SW. The SWM, per that thread, does the work and sends the information to the SWR. The data is stored in the profile directory.

2. but what about the other processes? We should definitely propagate this information.
SWM is already connected to PBackground. I guess the SWR should send messages to all the children.

I NI nsm to have a feedback. If he agrees, I can implement this.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> :ehsan, initially about:sw was running only on the parent process. But then
> we tried (during the work-week in Madrid) to expose this page to b2g. Here
> we need to have it running in the child process in order to have it
> accessible from the browser app.
> 
> I don't see why we cannot have about:sw in the child process. What is
> missing is the propagation of the unregistration/registration information.
> Tell me if I'm wrong but:
> 
> 1. ChildProcess wants to unregister a SW. The SWM, per that thread, does the
> work and sends the information to the SWR. The data is stored in the profile
> directory.
> 
> 2. but what about the other processes? We should definitely propagate this
> information.
> SWM is already connected to PBackground. I guess the SWR should send
> messages to all the children.
> 
> I NI nsm to have a feedback. If he agrees, I can implement this.

Is it possible and common for b2g to have the same origin loaded in different processes?
If it is this is going to be a good amount of work (see next paragraph). Otherwise, step 1 is all that matters, since we don't need the propagation, correct?

If we sync between processes, we'll have to fire relevant events/resolve promises to content as things change. This will be a fair amount of code that may not always agree with the spec. It might make more sense to hoist ServiceWorkerManager (or something like it) into the parent process and have it be the sole arbiter of state, with some sort of lightweight IPDL based proxy the only thing on the child process (which is mainly concerned with creating relevant WebIDL objects, or firing events on the right targets). This will be a fair amount of code too, and ideally I'd prefer not to block shipping on this.

Comment 12

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #11)
> (In reply to Andrea Marchesini (:baku) from comment #10)
> > :ehsan, initially about:sw was running only on the parent process. But then
> > we tried (during the work-week in Madrid) to expose this page to b2g. Here
> > we need to have it running in the child process in order to have it
> > accessible from the browser app.
> > 
> > I don't see why we cannot have about:sw in the child process. What is
> > missing is the propagation of the unregistration/registration information.
> > Tell me if I'm wrong but:
> > 
> > 1. ChildProcess wants to unregister a SW. The SWM, per that thread, does the
> > work and sends the information to the SWR. The data is stored in the profile
> > directory.
> > 
> > 2. but what about the other processes? We should definitely propagate this
> > information.
> > SWM is already connected to PBackground. I guess the SWR should send
> > messages to all the children.
> > 
> > I NI nsm to have a feedback. If he agrees, I can implement this.
> 
> Is it possible and common for b2g to have the same origin loaded in
> different processes?
> If it is this is going to be a good amount of work (see next paragraph).
> Otherwise, step 1 is all that matters, since we don't need the propagation,
> correct?

I prefer doing just step 1.  about:sw is only a stop-gap measure until we have proper devtools support for service workers, so I don't think we should do anything except for the bare minimum amount of work to get it to work on b2g.

Comment 13

[(no longer active)]

I talked to Andrea about this on IRC.  Given the fact that the about:serviceworker page would be loaded in the browser app process on b2g, we'd need to do something complicated here such as going from there to the parent process, then in the parent finding the child containing the registration and getting it to update/unregister it, and so on.

How badly do we need about:serviceworkers on b2g?  Given that we have added it as a very crude tool for people before proper support for service workers in devtools, I kind of prefer us to not spend a lot of time to fix it here and instead focus on more important stuff on our plate.

José, can we live without this on b2g?  (If not, we can fix this of course, it's just a matter of priorities!)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

about:serviceworkers is being added to b2g as a settings panel under the developer menu. You can see the details in bug 1155758 and an explanation about why I had to discard loading this in the browser at https://bugzilla.mozilla.org/show_bug.cgi?id=1155758#c3

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13)
> I talked to Andrea about this on IRC.  Given the fact that the
> about:serviceworker page would be loaded in the browser app process on b2g,
> we'd need to do something complicated here such as going from there to the
> parent process, then in the parent finding the child containing the
> registration and getting it to update/unregister it, and so on.

Thanks for investigating this.

> How badly do we need about:serviceworkers on b2g?

Fernando provided some information at comment 14.

> Given that we have added
> it as a very crude tool for people before proper support for service workers
> in devtools, I kind of prefer us to not spend a lot of time to fix it here
> and instead focus on more important stuff on our plate.

Sure, I don't want you guys to waste time on this.

Comment 16

[(no longer active)]

Sounds great, thanks guys!  I'll close this for now, please feel free to reopen if you needed something from the Gecko side.  :-)

Comment 17

[(no longer active)]

I was discussing this with bkelly on IRC the other day in the context of bug 1167325.  Since we may very well ship SW without any other devtools story, we may have to end up doing the work to make this page useful in e10s mode...

Sorry for the bad news, Andrea!  Do you have cycles for this?

Comment 18

[Andrea Marchesini [:baku]]

Yes, I hope to submit a patch for this week.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: asw.patch

Fixing this bug I realized we have to do something more for the propagation of registrations to child processes. For instance here a scenario:

a. 2 tabs, 2 process: child1 and child2.
b. child1 opens foobar.com and this page registers a SW
c. child2 after a while, opens barfoo.net and this page opens an iframe with foobar.com.
d. we should use the SW for that iframe but child2 doesn't know it should.

Maybe I'm wrong, but it seems to me this is a quite easy to reproduce scenario.

For the IPDL part, I would like to have ehsan or bent to take a look.

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8611251 [details] [diff] [review]
asw.patch

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

This is a great patch except for synchronizing calls to register() which should be removed in my opinion.

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +137,5 @@
>  
>    // Testing
>    DOMString getScopeForUrl(in nsIPrincipal aPrincipal, in DOMString aPath);
>  
> +  // Note: This is meant to be used only by about:serviceworkers.

Nit: No need for 'Note: '

@@ +138,5 @@
>    // Testing
>    DOMString getScopeForUrl(in nsIPrincipal aPrincipal, in DOMString aPath);
>  
> +  // Note: This is meant to be used only by about:serviceworkers.
> +  //It returns an array of nsIServiceWorkerInfo.

Nit: // It

@@ +143,5 @@
>    nsIArray getAllRegistrations();
>  
> +  // Note: This is meant to be used only by about:serviceworkers.
> +  // It calls softUpdate() for each child process.
> +  void propagateSoftUpdate(in unsigned long aAppId, in boolean aIsInBrowserElement,

Can we use OriginAttributes here? Only if there is an easy way to integrate them with XPIDL.

::: dom/workers/ServiceWorkerManager.cpp
@@ +272,5 @@
>  NS_INTERFACE_MAP_END
>  
> +namespace {
> +
> +ServiceWorkerManager* mSWMInstanceInitializing = nullptr;

Please use a g prefix instead of m.

::: dom/workers/ServiceWorkerManagerChild.cpp
@@ +17,5 @@
> +
> +bool
> +ServiceWorkerManagerChild::RecvNotifyRegister(
> +                                     const ServiceWorkerRegistrationData& aData)
> +{

Main thread assertions here and in the rest of this file.

@@ +25,5 @@
> +
> +  nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +  MOZ_ASSERT(swm);
> +
> +  swm->LoadRegistration(aData);

I don't agree with this. If there is an existing registration in this process, we will end up overwriting it and having an inconsistent state. There isn't a reasonable way to solve this as long as each child maintains its own state. The situation you outlined in comment 20, can currently only happen in b2g and I think I'd rather have child b not be controlled than introduce races due to trying to sync them.

The correct fix is to have the canonical state in the parent and the children always use IPC to get the state. This is something we should hash out in whistler since it will affect other things including network interception.

I think we should use this code only to facilitate about:serviceworkers working. Which means only support syncing softUpdate (which is non-destructive) and unregister (which is idempotent) for now.

Comment 22

[Andrea Marchesini [:baku]]

> its own state. The situation you outlined in comment 20, can currently only
> happen in b2g and I think I'd rather have child b not be controlled than
> introduce races due to trying to sync them.

I can easily reproduce this issue on e10s desktop build. I think that what you are saying is correct, but it's very rare to reproduce: 2 pages, loading the same origin trying to register a SW at the same time. Am I wrong?
What about if we proceed with this and in whistler we find a better solution to avoid this race condition?

Comment 23

[Andrea Marchesini [:baku]]

Attachment: asw.patch

rebased

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrea Marchesini (:baku) from comment #22)
> > its own state. The situation you outlined in comment 20, can currently only
> > happen in b2g and I think I'd rather have child b not be controlled than
> > introduce races due to trying to sync them.
> 
> I can easily reproduce this issue on e10s desktop build. I think that what
> you are saying is correct, but it's very rare to reproduce: 2 pages, loading
> the same origin trying to register a SW at the same time. Am I wrong?
> What about if we proceed with this and in whistler we find a better solution
> to avoid this race condition?

How did you reproduce on e10s when there is only one content process by default? Is one of the SWMs running in the parent?

Comment 25

[Andrea Marchesini [:baku]]

Attachment: asw.patch

bholley, this patch has been fully reviewed by nsm but I need you to take a look at the OriginAttributes serialization on IPC.

nsm: this patch is a rebased version of the previous one.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: asw.patch

Read the previous comment. I forgot a hg qref.

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8612794 [details] [diff] [review]
asw.patch

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

::: caps/BasePrincipalIPC.h
@@ +18,5 @@
> +
> +  static void Write(Message* aMsg, const paramType& aParam)
> +  {
> +    WriteParam(aMsg, aParam.mAppId);
> +    WriteParam(aMsg, aParam.mInBrowser);

I'm concerned about the propagation of all of these serialization routines. We have OriginAttributes::CreateSuffix, OriginAttributes::Serialize, PrincipalToPrincipalInfo, and now this.

The first three are kind of necessary, I guess, but it seems like this 4th thing should at least share code with PrincipalToPrincipalInfo. I'd also like the implementation to live immediately next to OriginAttributes::Serialize and friends, so that it's very clear what needs to get updated when new attributes are added.

The nicest thing, I think, would be to implemented OriginAttributes::FromSuffix (as you suggested on IRC), and then switch all the other serializations to be built on top of that. I'll leave it up to you whether to do that or not. :-)

Comment 28

[Andrea Marchesini [:baku]]

Attachment: asw.patch

Comment 29

[Andrea Marchesini [:baku]]

Attachment: interdiff

I unified the parsing/decoding of OriginAttributes using URLSearchParams.

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8613395 [details] [diff] [review]
interdiff

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

::: caps/BasePrincipal.cpp
@@ +43,5 @@
> +  usp->Serialize(value);
> +
> +  aStr.Truncate();
> +  aStr.AppendLiteral("!");
> +  aStr.Append(NS_ConvertUTF16toUTF8(value));

I changed this code in:

  aStr.Truncate();                                                                                                                             
                                                                                                                                               
  usp->Serialize(value);                                                                                                                       
  if (!value.IsEmpty()) {                                                                                                                      
    aStr.AppendLiteral("!");                                                                                                                   
    aStr.Append(NS_ConvertUTF16toUTF8(value));                                                                                                 
  }

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrea, there is one bug in the code.

register() in script leads to a call to SWM::StoreRegistration(), as does activation and other times when we change the registration state in memory and want to reflect it on disk. StoreRegistration() propagates the register() to all children (including the sending child!) where LoadRegistration() again calls CreateNewRegistration() (please add an assertion in CreateNewRegistration() that prevents this!), even though that registration already exists in the sending child. I think you just need to re-use the registration but update its fields in such a case.

@@ -2707,18 +2709,22 @@ ServiceWorkerManager::LoadRegistration(
   AssertIsOnMainThread();
 
   nsCOMPtr<nsIPrincipal> principal =
     PrincipalInfoToPrincipal(aRegistration.principal());
   if (!principal) {
     return;
   }
 
-  ServiceWorkerRegistrationInfo* registration =
-    CreateNewRegistration(aRegistration.scope(), principal);
+  nsRefPtr<ServiceWorkerRegistrationInfo> registration =
+    GetRegistration(principal, aRegistration.scope());
+  if (!registration) {
+    fprintf(stderr, "CreateNewReg from LoadRegistration\n");
+    registration = CreateNewRegistration(aRegistration.scope(), principal);
+  }
 
   registration->mScriptSpec = aRegistration.scriptSpec();
 
   const nsCString& currentWorkerURL = aRegistration.currentWorkerURL();
   if (!currentWorkerURL.IsEmpty()) {
     registration->mActiveWorker =
       new ServiceWorkerInfo(registration, currentWorkerURL, aRegistration.activeCacheName());
     registration->mActiveWorker->SetActivateStateUncheckedWithoutEvent(ServiceWorkerState::Activated);

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8613395 [details] [diff] [review]
interdiff

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

Does BasePrincipalIPC.h really need to exist, or can we put this with the other principal-related IPC code? Seems like that stuff should live in one place.

Looks good in general, I just want to look at the final product again.

::: caps/BasePrincipal.cpp
@@ +43,5 @@
> +  usp->Serialize(value);
> +
> +  aStr.Truncate();
> +  aStr.AppendLiteral("!");
> +  aStr.Append(NS_ConvertUTF16toUTF8(value));

>  changed this code in:

Yeah, definitely need that. :-)

So. One thing we need with the suffixes is an ordering that is both persistent _and_ canonical, because these strings will end up in indexedDB databases etc, and we need to be able to always compare them to determine whether things are same-origin or not.

So the usage of URLSearchParams here is great, but only if we're certain that the current behavior of URLSearchParams::Serialize (that the ordering matches the ordering of Set() calls) is guaranteed to hold. Can you add a comment to both URLSearchParams::Set and URLSearchParams::Serialize to indicate that we're depending on that from CAPS?

@@ +50,5 @@
> +bool
> +OriginAttributes::PopulateFromSuffix(const nsACString& aStr)
> +{
> +  nsRefPtr<URLSearchParams> usp = new URLSearchParams();
> +  usp->ParseInput(aStr, nullptr);

Hm, does ParseInput know how to handle the leading |!|?

Indeed, I think we actually want to _enforce_ that the leading |!| exists - please do so explicitly.

@@ +53,5 @@
> +  nsRefPtr<URLSearchParams> usp = new URLSearchParams();
> +  usp->ParseInput(aStr, nullptr);
> +
> +  uint32_t appId = nsIScriptSecurityManager::NO_APP_ID;
> +  bool inBrowser = false;

Please don't do this - these defaults are already given in the dictionary definition in the .webidl, and we don't want to duplicate them.

@@ +77,5 @@
> +      return false;
> +    }
> +
> +    inBrowser = true;
> +  }

I think this method should also fail if there are any keys that don't match the ones we're parsing here. One way to do this would be to count the number of "hits" and require that the number of unique keys in the set is equal to that number.

@@ +80,5 @@
> +    inBrowser = true;
> +  }
> +
> +  mAppId = appId;
> +  mInBrowser = inBrowser;

Per above, please remove this and just set the member variables directly in the branches.

@@ +86,4 @@
>  }
>  
>  void
>  OriginAttributes::Serialize(nsIObjectOutputStream* aStream) const

Given this, we can get rid of the OriginAttributes::{Des,S}erialize methods and just inline this implementation into the one caller, I think.

You also need to bump the CIDs for all the classes that serialize these attributes - so NS_NULLPRINCIPAL_CID, NS_PRINCIPAL_CID, and NS_EXPANDEDPRINCIPAL_CID.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: asw.patch

I'll fix the nsm's comments in a separate interdiff.

Comment 34

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 8613934 [details] [diff] [review]
asw.patch

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

This is missing the requested comment in URLSearchParams.cpp.

::: caps/BasePrincipal.cpp
@@ +114,5 @@
> +  nsRefPtr<URLSearchParams> usp = new URLSearchParams();
> +  usp->ParseInput(Substring(aStr, 1, aStr.Length() - 1), nullptr);
> +
> +  bool status = true;
> +  usp->ForEach(PopulateFromSuffixIterator, &status);

This sure looks like a big memory hazard to me. Did you test this patch?

void*'s suck for precisely this reason. How about making ParamFunc a template parameter in URLSearchParams.h and doing this with lambda capture instead? That'd get rid of the boilerplate too. I'd also be fine with reverting to the previous approach and adding the counter.

::: caps/nsNullPrincipal.cpp
@@ +163,5 @@
> +  nsAutoCString suffix;
> +  nsresult rv = aStream->ReadCString(suffix);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!mOriginAttributes.PopulateFromSuffix(suffix)) {

Please use |bool ok| analogous to |nsresult rv| So that we can use NS_ENSURE for both.

@@ +176,5 @@
>  {
> +  nsAutoCString suffix;
> +  OriginAttributesRef().CreateSuffix(suffix);
> +  nsresult rv = aStream->WriteStringZ(suffix.get());
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

You can just NS_ENSURE_SUCCESS here.

::: caps/nsPrincipal.cpp
@@ +361,5 @@
>    domain = do_QueryInterface(supports);
>  
> +  nsAutoCString suffix;
> +  rv = aStream->ReadCString(suffix);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

NS_ENSURE.

@@ +369,2 @@
>    OriginAttributes attrs;
> +  if (!attrs.PopulateFromSuffix(suffix)) {

bool ok.

@@ +413,5 @@
>  
> +  nsAutoCString suffix;
> +  OriginAttributesRef().CreateSuffix(suffix);
> +  rv = aStream->WriteStringZ(suffix.get());
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

NS_ENSURE.

Comment 36

[Andrea Marchesini [:baku]]

Attachment: asw.patch

bholley: I'm not a bit fan of C++ templates. Here your comments applied with a Iterator class.

nsm: I fixed the problem of propagating the notification to the parent who sent it using parentIDs. The main reason why I need this is bacuase parent classes are not refcnted and I don't want to keep them alive in the callback runnable.

Comment 37

[Bobby Holley (:bholley)]

Comment on attachment 8614581 [details] [diff] [review]
asw.patch

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

This is still missing the very important comment I asked for in comment 32 and again in comment 35. Please add it, and also add a comment in https://hg.mozilla.org/mozilla-central/annotate/b0a507af2b4a/caps/tests/unit/test_origin.js#l64 indicating that the ordering of serialized origin attributes is extremely important for persistent data, and must be preserved.

r=me with that.

Comment 38

[Andrea Marchesini [:baku]]

(In reply to Bobby Holley (:bholley) from comment #37)
> Comment on attachment 8614581 [details] [diff] [review]
> asw.patch
> 
> Review of attachment 8614581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is still missing the very important comment I asked for in comment 32
> and again in comment 35. Please add it, and also add a comment in
> https://hg.mozilla.org/mozilla-central/annotate/b0a507af2b4a/caps/tests/unit/
> test_origin.js#l64 indicating that the ordering of serialized origin
> attributes is extremely important for persistent data, and must be preserved.
> 
> r=me with that.

ah yes. sorry. forgot to write a comment about this: the fact is that we have a mochitest just for this.
URLSearchParams must respect the order of attributes. But, yes, I'll add a specific comment before landing.

Comment 39

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini (:baku) from comment #38)
> ah yes. sorry. forgot to write a comment about this: the fact is that we
> have a mochitest just for this.
> URLSearchParams must respect the order of attributes. But, yes, I'll add a
> specific comment before landing.

Great! Thank you. :-)

Comment 40

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8614581 [details] [diff] [review]
asw.patch

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

::: dom/workers/ServiceWorkerManagerParent.h
@@ +52,5 @@
> +
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> +
> +  nsRefPtr<ServiceWorkerManagerService> mService;
> +  uint64_t mID;

Just a comment that this is used to ignore messages sent by ourselves.

Comment 41

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Using URLSearchParams directly prevents OriginAttributes from being used on non-CCed threads. In dom/cache, I want to use OriginAttributes CreateSuffix/PopulateFromSuffix to write the suffix to IDB. Could we split the actual string manipulation into helpers?

Comment 42

[Andrea Marchesini [:baku]]

do you mind if we do this in a follow up?

Comment 43

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Sure followup is fine, I just wanted it noted.

Comment 44

[(no longer active)]

Bug 1171486 may bitrot this...

Comment 45

[Andrea Marchesini [:baku]]

Attachment: asw.patch

Comment 46

[Andrea Marchesini [:baku]]

Comment on attachment 8614581 [details] [diff] [review]
asw.patch

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

Comment 47

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

need to rebase after Bug 1171486

Comment 48

[Andrea Marchesini [:baku]]

Attachment: 247285.diff

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

Comment 49

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc305c9b5d05

Comment 50

[Carsten Book [:Tomcat]]

sorry backout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10471606&repo=mozilla-inbound

Comment 51

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/411cbf52e73f

Comment 52

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Update about:sw for b2g

about:sw was broken on b2g since bug 1162088, but this patch should fix the orange temporarily.

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

I filed bug 1171915 and bug 1171917 to properly fix about:sw in b2g and add better tests to catch this kind of regressions.

Comment 53

[José Antonio Olivera Ortega [:jaoo]]

FYI while working on bug 1171915 I've seen this:

Assertion failure: data, at /Volumes/firefoxos/dev/mozilla-central/xpcom/base/nsCycleCollector.cpp:3962
#01: mozilla::OriginAttributes::CreateSuffix(nsACString_internal&) const[/Volumes/firefoxos/dev/mozilla-central/objdir-desktop/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb57745]

Comment 54

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #53)
> FYI while working on bug 1171915 I've seen this:
> 
> Assertion failure: data, at
> /Volumes/firefoxos/dev/mozilla-central/xpcom/base/nsCycleCollector.cpp:3962
> #01: mozilla::OriginAttributes::CreateSuffix(nsACString_internal&)
> const[/Volumes/firefoxos/dev/mozilla-central/objdir-desktop/dist/
> NightlyDebug.app/Contents/MacOS/XUL +0xb57745]

It seems it's a know issue, see bug 1172542.

Comment 55

[Noemí Freire (:noemi)]

Per comment 54 adding Bug 1172542 dependency.

Comment 56

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #54)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #53)

> It seems it's a know issue, see bug 1172542.

I was wrong, the crash is different. See it at https://pastebin.mozilla.org/8836358.

Comment 57

[Fernando Jiménez Moreno [:ferjm]]

Try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2426dd39cfc

Comment 58

[Fernando Jiménez Moreno [:ferjm]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #56)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #54)
> > (In reply to José Antonio Olivera Ortega [:jaoo] from comment #53)
> 
> > It seems it's a know issue, see bug 1172542.
> 
> I was wrong, the crash is different. See it at
> https://pastebin.mozilla.org/8836358.

It seems that this will be fixed by bug 1169044

Comment 59

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4444e384ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb48faca184

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc4444e384ba
https://hg.mozilla.org/mozilla-central/rev/2cb48faca184

Comment 61

[Noemí Freire (:noemi)]

Hi,

Please see below the behavior seen while checking about:sw under e10s and non-e10s scenarios on desktop with today's (6/11) master build:
*e10s
**unregister button - it works as expected (fixed!). The Service Worker is properly unregistered
**update button - an exception occurs and the browser is automatically closed when clicking on the update button. In the following link you can see the traces: https://pastebin.mozilla.org/8836501. I also attach the corresponding crash report "Crash_Report_6_11_Update_Button"
*non-e10s
**unregister button - it works as expected. The Service Worker is properly unregistered
**update button - nothing occurs when clicking on this button, no traces at all

ni to Andrea to see how to handle this, let me know if a new bug should be created. Thanks!

Regarding B2G scenario, I will wait for Bug 1171915 to be landed to check it

Comment 62

[Noemí Freire (:noemi)]

Attachment: Crash_Report_6_11_Update_Button

Comment 63

[Noemí Freire (:noemi)]

mmm, I didn't realized about comment 58, it seems the issue raised in comment 61 and comment 62 will be fixed in Bug 1169044. Maintaining ni to Andrea just to confirm it. Thanks!

Comment 64

[Andrea Marchesini [:baku]]

Yes, I confirm. that fix should land soon.

Comment 65

[Noemí Freire (:noemi)]

Hi,

Once Bug 1169044 has been landed in master, we've checked the unregister/update behavior in about:sw under e10s and non-e10s scenarios. Tests made on desktop with today's (6/29) master build (eaf4f9b45117 revision):
*e10s
**unregister button - it works as expected. The Service Worker is properly unregistered
**update button - 
***STRs
1- register a sw
2- open about:sw
3- click on the update button within about:sw. It works!!
***STRs:
1- open about:sw (no sw shown which is correct)
2- register a sw
3- go to about:sw page (the sw is properly shown), then, click on the update button, nothing happens (This issue has been gathered under Bug 1178236)

*non-e10s
**unregister button - it works as expected. The Service Worker is properly unregistered
**update button - nothing occurs when clicking on this button, no traces at all (Bug 1178233)

Regarding B2G scenario, I will wait for gaia patch within Bug 1171915 to be landed to check it. Thanks!