Closed Bug 1138793 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: kershaw, Assigned: kershaw)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Blocks: nested-oop
Hi baku,

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

Thanks.
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(kechang)
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.
(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
Flags: needinfo?(kechang)
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.)
Flags: needinfo?(kechang)
(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?
Flags: needinfo?(kechang)
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.
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Assignee: nobody → kechang
Ben, could you please take a look at this patch?
I just add a IPC bounce in PBackground for getting the browser configuration.
Thanks.
Attachment #8575883 - Flags: review?(bkelly)
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.
Attachment #8575883 - Flags: review?(bkelly) → feedback-
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.
Flags: needinfo?(bent.mozilla)
(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.
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.
Flags: needinfo?(bent.mozilla)
Attachment #8575883 - Flags: feedback- → review?(amarchesini)
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?
Flags: needinfo?(amarchesini) → needinfo?(kechang)
Attachment #8575883 - Flags: review?(amarchesini)
(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?
Flags: needinfo?(kechang)
Andrea, may I know your feedback and the suggestions about how to fix this bug?
Thanks.
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
(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.
Flags: needinfo?(amarchesini)
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.
Attachment #8575883 - Attachment is obsolete: true
Attachment #8585356 - Flags: review?(amarchesini)
Hi Andrea, that would be great if you can provide feedback and review for the patch, thanks!
Flags: needinfo?(amarchesini)
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
Attachment #8585356 - Flags: review?(amarchesini) → review+
(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?
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.
Flags: needinfo?(amarchesini)
Andrea, please take a look at this patch. I've added a ipdlh for BrowserConfiguration. Thanks.
Attachment #8585356 - Attachment is obsolete: true
Attachment #8593194 - Flags: review?(amarchesini)
Update the patch since the last one missed BrowserConfiguration.ipdlh.
Attachment #8593194 - Attachment is obsolete: true
Attachment #8593194 - Flags: review?(amarchesini)
Attachment #8593260 - Flags: review?(amarchesini)
Hi Andrea, would you mind taking a look at this patch? It's just a small change compared to the previous version. Thanks.
Flags: needinfo?(amarchesini)
Comment on attachment 8593260 [details] [diff] [review]
Add a sync IPC to get BrowserConfiguration - v2

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

lgtm!
Attachment #8593260 - Flags: review?(amarchesini) → review+
Carry reviewer's name.
Attachment #8593260 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8596995 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8963c3c2c56
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.