1138793 - [nested_oop] ServiceWorkerRegistrar::Get() is not working in content process

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Kershaw Chang [:kershaw]]

Since TabParent may live in content process now, ServiceWorkerRegistrar::Get()[1] will cause the process to crash due to [2].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#727
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegistrar.cpp#71

Comment 1

[Kershaw Chang [:kershaw]]

Hi baku,

Could you please take a look at comment #1?
Is it possible to get ServiceWorkerRegistrar in content process?

Thanks.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Kershaw, how do we expect this sort of thing to work for nested content processes?  None of the IO that ServiceWorkerRegistrar does will work in the the content process.  The ServiceWorker Cache will have a similar problem.  Does IndexedDB work in nested content processes?

It seems each feature we want to work in nested content processes must be manually proxied through until we hit the parent process?  That seems like a lot of plumbing to do.

Do we really need all these features in nested content processes?  I don't even know how to test that case.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

It seems maybe PBackground should handle this automatically at the IPC layer instead of requiring each feature to manually pipe nested content process requests through to the parent process.

Comment 4

[Kershaw Chang [:kershaw]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Kershaw, how do we expect this sort of thing to work for nested content
> processes?  None of the IO that ServiceWorkerRegistrar does will work in the
> the content process.  The ServiceWorker Cache will have a similar problem. 
> Does IndexedDB work in nested content processes?
> 
We already have bug 1020196 to let IndexedDB work in nested content process. As I known, the reason why IndexedDB cannot work in nested content process now is that PIndexedDBPermissionRequest is managed by PBrowser. To let PIndexedDBPermissionRequest be managed by PContent should allow nested content process to use IndexedDB.
Since I am not familiar with ServiceWorker, I cannot tell how this should work in nested content process. Perhaps PBackground you mentioned below is the answer?

> It seems each feature we want to work in nested content processes must be
> manually proxied through until we hit the parent process?  That seems like a
> lot of plumbing to do.
> 
> Do we really need all these features in nested content processes?  I don't
> even know how to test that case.
I can only say it would be great to let all features work in nested content process.
We have --nested_oop option for running mochitest in nested content process. For example, to run dom/workers/test in nested content process, just run this command: ./mach mochitest-plain --nested_oop dom/workers/test

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Kershaw, I believe both the SW registration and the SW Cache are managed by PBackground.  PBackground is managed by PContent.  Should that allow this to work?  Do we need to do something particular in our code?  Or does PBackground need some fixup to support this?  (Note, IDB now uses PBackground as well.)

Comment 6

[Kershaw Chang [:kershaw]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Kershaw, I believe both the SW registration and the SW Cache are managed by
> PBackground.  PBackground is managed by PContent.  Should that allow this to
> work?  Do we need to do something particular in our code?  Or does
> PBackground need some fixup to support this?  (Note, IDB now uses
> PBackground as well.)

Please take a look at comment #1. The problem is that ServiceWorkerRegistrar::Get() may run in content process. Is it possible to let ServiceWorkerRegistrar also be available in content process?

Comment 7

[Kershaw Chang [:kershaw]]

It seems that TabParent only uses ServiceWorkerRegistrar to get BrowserConfiguration. Can we change it to get BrowserConfiguration via IPC if the TabParent is in content process?

Ben, may I know your thought? Thanks.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Probably, but we'd have to do this extra IPC bounce in every feature right?  Cache would need it, etc.

Personally I think PBackground should always go to the root parent process regardless of nested status, etc.  The whole point of using PBackground is that you don't have to worry about what process the "child" is in.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Get browser configuration in PBackground

Ben, could you please take a look at this patch?
I just add a IPC bounce in PBackground for getting the browser configuration.
Thanks.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8575883 [details] [diff] [review]
Get browser configuration in PBackground

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

Sorry, I'm not a peer so you will need Andrea's review.

In any case, I really dislike the architecture here. As I said in a previous comment, I think we should make PBackground in a nested content process go to the root chrome process always.  Then all of this will just work.

I mean, after this patch are we going to have another extra IPC bounce added to Cache?  Most ServiceWorkers will want to use Cache which will fail because QuotaManager and files can't be opened in a child process.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Ben, how do you think PBackground should work for nested content processes?

It seems to me the advantage of using PBackground is that it lets you communicate with the root chrome process background thread without worrying about what current process you are in.  Therefore, I would argue PBackground messages should automatically be routed to the root chrome process.

Comment 12

[Kershaw Chang [:kershaw]]

(In reply to Ben Kelly [:bkelly] from comment #10)
> Comment on attachment 8575883 [details] [diff] [review]
> Get browser configuration in PBackground
> 
> Review of attachment 8575883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I'm not a peer so you will need Andrea's review.
> 
> In any case, I really dislike the architecture here. As I said in a previous
> comment, I think we should make PBackground in a nested content process go
> to the root chrome process always.  Then all of this will just work.
> 
Currently, PBackground in nested content process is connecting to root chrome process directly. The nested content process still has a PContent that can send/recv messages to parent process directly.

> I mean, after this patch are we going to have another extra IPC bounce added
> to Cache?  Most ServiceWorkers will want to use Cache which will fail
> because QuotaManager and files can't be opened in a child process.

The main difference between nested content process and normal content process is that the nested iframe's TabParent is in content process, not in parent process. The problem of this bug is that it seems BrowserConfiguration (ServiceWorkerRegistrationData) is only available in parent process. We can not get it in TabParent:::InitBrowserConfiguration if the TabParent is in content process. So, it seems to me the only way to fix this is making nested content process to get BrowserConfiguration via IPC.
I am not sure about the Cache case. It seems that Cache has nothing to do with TabParent/TabChild in my understanding, so I think Cache should be already working in nested content process.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Ok, sorry for my confusion.  I think its best to ask Andrea for review as the author of the code and as a DOM peer.

Comment 14

[Andrea Marchesini [:baku]]

I have a question first. Then ask me a review.

1. What prevents that registration of a new serviceWorker before having all the content fully loaded from the PBackground thread?
2. What prevents a content is loaded before the ServiceWorkerManager receives data from the PBackground thread?

Comment 15

[Kershaw Chang [:kershaw]]

(In reply to Andrea Marchesini (:baku) from comment #14)
> I have a question first. Then ask me a review.
> 
> 1. What prevents that registration of a new serviceWorker before having all
> the content fully loaded from the PBackground thread?
> 2. What prevents a content is loaded before the ServiceWorkerManager
> receives data from the PBackground thread?

To be honest, I have no idea about your questions since this is my first time touching ServiceWorker and PBackground. But, I would like to try to answer.

1. It seems that mServiceWorkerRegistrationInfos must have data first before the registration of a new ServiceWorker? Is this the reason that the registration data needs to be sent to content process by SendLoadURL?
2. Not sure about this. Perhaps we have to wait until PBackgroundChild actor is created?

Comment 16

[Kershaw Chang [:kershaw]]

Andrea, may I know your feedback and the suggestions about how to fix this bug?
Thanks.

Comment 17

[Andrea Marchesini [:baku]]

Sorry I was in PTO.

> 1. It seems that mServiceWorkerRegistrationInfos must have data first before
> the registration of a new ServiceWorker? Is this the reason that the
> registration data needs to be sent to content process by SendLoadURL?

Right. This is the reason why we have data sent by LoadURL(). The registration infos must be there before any loading.

Probably the way we can fix this issue is that we postpone the loadURL until the data is fully loaded in the TabParent in the child process. We can use PBackground for this.

Comment 18

[Kershaw Chang [:kershaw]]

(In reply to Andrea Marchesini (:baku) from comment #17)
> Right. This is the reason why we have data sent by LoadURL(). The
> registration infos must be there before any loading.
> 
> Probably the way we can fix this issue is that we postpone the loadURL until
> the data is fully loaded in the TabParent in the child process. We can use
> PBackground for this.

How about adding a sync IPC in PContent to get registration infos if the TabParent is in child process? I think using PContent instead of PBackground is more easier since we don't have to take care of switching between main thread and background thread.
Or can we just send the old registration infos that is already existed in child process to nested child process?

Thanks.

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: Add a sync IPC to get BrowserConfiguration

Andrea, could you please take a look at this patch?
I think adding a sync IPC to get the browser configuration is the most straightforward way for this issue. Does this make sense to you?
Thanks.

Comment 20

[howie [:howie]]

Hi Andrea, that would be great if you can provide feedback and review for the patch, thanks!

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8585356 [details] [diff] [review]
Add a sync IPC to get BrowserConfiguration

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

lgtm!

::: dom/ipc/ContentParent.cpp
@@ +4761,5 @@
>  }
>  
> +bool
> +ContentParent::RecvGetBrowserConfiguration(const nsCString& aURI, BrowserConfiguration* aConfig)
> +{

MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default)

Do we support contentParent into a contentParent into a contentParent?

::: dom/ipc/PBrowser.ipdl
@@ -67,5 @@
>    CommandInt[] multiLineCommands;
>    CommandInt[] richTextCommands;
>  };
>  
> -struct BrowserConfiguration

why do you move this? I prefer to have in PBrowser.ipdl because the idea is that we can add more things here in case we need.

::: dom/ipc/TabParent.cpp
@@ +14,2 @@
>  #include "mozilla/dom/PContentPermissionRequestParent.h"
> +#include "mozilla/dom/ContentParent.h"

alphabetic order.

::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh
@@ +16,5 @@
>    nsCString currentWorkerURL;
>    PrincipalInfo principal;
>  };
>  
> +struct BrowserConfiguration

move this back to PBrowser.ipdl

Comment 22

[Kershaw Chang [:kershaw]]

(In reply to Andrea Marchesini (:baku) from comment #21)

Thanks for reviewing this.

> 
> lgtm!
> 
> ::: dom/ipc/ContentParent.cpp
> @@ +4761,5 @@
> >  }
> >  
> > +bool
> > +ContentParent::RecvGetBrowserConfiguration(const nsCString& aURI, BrowserConfiguration* aConfig)
> > +{
> 
> MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default)
> 
> Do we support contentParent into a contentParent into a contentParent?

No, we don't. I will add this at next version.

> ::: dom/ipc/PBrowser.ipdl
> @@ -67,5 @@
> >    CommandInt[] multiLineCommands;
> >    CommandInt[] richTextCommands;
> >  };
> >  
> > -struct BrowserConfiguration
> 
> why do you move this? I prefer to have in PBrowser.ipdl because the idea is
> that we can add more things here in case we need.

If we don't do this, we have to declare BrowserConfiguration again in PContent.ipdl. I think we don't want to repeat this again, right?
Besides moving the declaration of BrowserConfiguration to a .ipdlh file, do we have another way to do this?

Comment 23

[Andrea Marchesini [:baku]]

Can you create a ipdlh file just for the BrowserConfiguration? Otherwise keep what you did. When we have something more to add to that struct, we can find a better way. Thanks.

Comment 24

[Kershaw Chang [:kershaw]]

Attachment: Add a sync IPC to get BrowserConfiguration - v2

Andrea, please take a look at this patch. I've added a ipdlh for BrowserConfiguration. Thanks.

Comment 25

[Kershaw Chang [:kershaw]]

Attachment: Add a sync IPC to get BrowserConfiguration - v2

Update the patch since the last one missed BrowserConfiguration.ipdlh.

Comment 26

[Kershaw Chang [:kershaw]]

Test run looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e024dbd3587

Comment 27

[Kershaw Chang [:kershaw]]

Hi Andrea, would you mind taking a look at this patch? It's just a small change compared to the previous version. Thanks.

Comment 28

[Andrea Marchesini [:baku]]

Comment on attachment 8593260 [details] [diff] [review]
Add a sync IPC to get BrowserConfiguration - v2

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

lgtm!

Comment 29

[Kershaw Chang [:kershaw]]

Attachment: Add a sync IPC to get BrowserConfiguration - v2, r=baku

Carry reviewer's name.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8963c3c2c56