1020186 - Make POfflineCacheUpdate be managed by PContent

Status: RESOLVED FIXED

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

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

nsOfflineCacheUpdateService is chrome process only. Actually TabParent doesn't handle any offline cache related thing so it make sense to move it to PContent.

Comment 1

[Kershaw Chang [:kershaw]]

Hi Kan-Ru,

I think this bug is depended on bug 1020157, because the constructor of OfflineCacheUpdateParent needs OwnOrContainingAppId from ContentParent.
I would like to work this after bug 1020157 is landed. What do you think?

Thanks.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kershaw Chang [:kershaw] from comment #1)
> Hi Kan-Ru,
> 
> I think this bug is depended on bug 1020157, because the constructor of
> OfflineCacheUpdateParent needs OwnOrContainingAppId from ContentParent.
> I would like to work this after bug 1020157 is landed. What do you think?
> 
> Thanks.

Bug 1020157 has got all r+'ed but it depends on 1029352 so it may take sometime to land. I think you could go ahead and create patches for this bug with assumption there will be OwnOrContainingAppId in ContentParent.

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: Make POfflineCacheUpdate be managed by PContent - v1

Patch V1.

Comment 4

[Kershaw Chang [:kershaw]]

Basically, it's just a straight move from PBrowser to PContent.
Note that we assume that OwnOrContainingAppId is already available in ContentParent (see also bug 1020157).

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8475747 [details] [diff] [review]
Make POfflineCacheUpdate be managed by PContent - v1

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

::: dom/ipc/ContentParent.cpp
@@ +3660,5 @@
> +    nsRefPtr<mozilla::docshell::OfflineCacheUpdateParent> update =
> +        new mozilla::docshell::OfflineCacheUpdateParent(OwnOrContainingAppId(),
> +                                                        mIsForBrowser);
> +    // Use this reference as the IPDL reference.
> +    return update.forget().take();

This assumes the one-process-one-app relationship. I think this will break when a process could contain multiple App as in bug 1020157. We should check that and maybe forbid offline cache?

Comment 6

[Kershaw Chang [:kershaw]]

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=d4a02ae933da

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #6)
> Try result:
> https://tbpl.mozilla.org/?tree=Try&rev=d4a02ae933da

I found some test cases failed due to the patch from bug 1020157. Please also take a look at the try run that only includes the patch from bug 1020157: https://tbpl.mozilla.org/?tree=Try&rev=d6dd1abaf6e2

After some investigation, I found mOpener should be also explicitly initialized to nullptr for the constructor used for nuwa case, otherwise b2g process will crash when opening the browser app.

Kan-Ru,
Could you please add the following change into your patch in bug 1020157?

@@ -1915,16 +1915,17 @@ FindFdProtocolFdMapping(const nsTArray<P
  *
  * For Nuwa.
  */
 ContentParent::ContentParent(ContentParent* aTemplate,
                              const nsAString& aAppManifestURL,
                              base::ProcessHandle aPid,
                              const nsTArray<ProtocolFdMapping>& aFds)
     : mAppManifestURL(aAppManifestURL)
+    , mOpener(nullptr)
     , mIsForBrowser(false)
     , mIsNuwaProcess(false)
 {
     InitializeMembers();  // Perform common initialization.

     sContentParents->insertBack(this);

     // From this point on, NS_WARNING, NS_ASSERTION, etc. should print out the

Thanks.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kershaw Chang [:kershaw] from comment #7)
> (In reply to Kershaw Chang [:kershaw] from comment #6)
> > Try result:
> > https://tbpl.mozilla.org/?tree=Try&rev=d4a02ae933da
> 
> I found some test cases failed due to the patch from bug 1020157. Please
> also take a look at the try run that only includes the patch from bug
> 1020157: https://tbpl.mozilla.org/?tree=Try&rev=d6dd1abaf6e2
> 
> After some investigation, I found mOpener should be also explicitly
> initialized to nullptr for the constructor used for nuwa case, otherwise b2g
> process will crash when opening the browser app.
> 
> Kan-Ru,
> Could you please add the following change into your patch in bug 1020157?
> 
> @@ -1915,16 +1915,17 @@ FindFdProtocolFdMapping(const nsTArray<P
>   *
>   * For Nuwa.
>   */
>  ContentParent::ContentParent(ContentParent* aTemplate,
>                               const nsAString& aAppManifestURL,
>                               base::ProcessHandle aPid,
>                               const nsTArray<ProtocolFdMapping>& aFds)
>      : mAppManifestURL(aAppManifestURL)
> +    , mOpener(nullptr)
>      , mIsForBrowser(false)
>      , mIsNuwaProcess(false)
>  {
>      InitializeMembers();  // Perform common initialization.
> 
>      sContentParents->insertBack(this);
> 
>      // From this point on, NS_WARNING, NS_ASSERTION, etc. should print out
> the
> 
> Thanks.

This is fixed in the Nuwa bug 1040561 and followup bug 1057230, they should be land together.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Make POfflineCacheUpdate be managed by PContent - v2

Hi Honza, Fabrice,

May I have your feedbacks about this patch?

Thanks.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 8477321 [details] [diff] [review]
Make POfflineCacheUpdate be managed by PContent - v2

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

I guess we're doing that for the nested OOP use cases? If so, I wonder how that fits schedule-wise with our plan to deprecate appcache support.

Comment 11

[Kershaw Chang [:kershaw]]

I think Kan-Ru knows more specific details than me.
Kan-Ru, would you please help to answer Fabrice's question?

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> Comment on attachment 8477321 [details] [diff] [review]
> Make POfflineCacheUpdate be managed by PContent - v2
> 
> Review of attachment 8477321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess we're doing that for the nested OOP use cases? If so, I wonder how
> that fits schedule-wise with our plan to deprecate appcache support.

By deprecating appcache do you mean we will stop exposing appache to the web content? I think this is not a question that I can answer..

Comment 13

[Kershaw Chang [:kershaw]]

I think we only have to disable appcache temporary only for the cases that dom.ipc.reuse_parent_app is enabled.
When we can get the correct app id from OwnOrContainingAppId in ContentParent, we can file another bug for reenabling appcache.
Please correct me if I am wrong. Thanks.

Comment 14

[[:fabrice] Fabrice Desré]

Well, that's really annoying. Breaking an app that wants to use appcache based on something that is totally undetectable is not good.

Let's do the things in the right order instead, fixing OwnOrContainingAppId and then landing this one.

Comment 15

[Kai-Chih Hu [:khu]]

Use feature-b2g:2.2? rather than just 2.2.

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Patch1: Revise the API in ContentProcessManager - v1

Hi Kyle,

Could you take a look at this patch?
It's about changing one API in ContentProcessManager to get a TabContext by the given ContentProcess and Tab ID.

Thanks.

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Make POfflineCacheUpdate be managed by PContent - v3

Hi Honza,

Please take a look at this patch. Since bug 1020172 is landed, we can get the correct app id in ContentParent now.

Thanks.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8518012 [details] [diff] [review]
Patch1: Revise the API in ContentProcessManager - v1

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

r=me

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8518023 [details] [diff] [review]
Patch 2: Make POfflineCacheUpdate be managed by PContent - v3

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

::: dom/ipc/PContent.ipdl
@@ +799,5 @@
> +     *   True if the update was initiated by a document load that referred
> +     *   a manifest.
> +     *   False if the update was initiated by applicationCache.update() call.
> +     * @param tabId
> +     *   To identify which tab owns the app.

Put this comment to the bottom of this whole comment block.  And next time read more carefully ;)

::: uriloader/prefetch/OfflineCacheUpdateChild.cpp
@@ +435,5 @@
>      // a reference to us. Will be released in RecvFinish() that identifies 
>      // the work has been done.
> +    ContentChild::GetSingleton()
> +        ->SendPOfflineCacheUpdateConstructor(this, manifestURI, documentURI,
> +                                             stickDocument, child->GetTabId());

I'd slightly more prefer the following formatting:

CCh::GS()->SPOCUC(
  this, mani...,
  stick... );

Comment 20

[Kershaw Chang [:kershaw]]

Attachment: Patch1: Revise the API in ContentProcessManager - v1, r=khuey

Rebase and carry reviewer's name.

Comment 21

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Make POfflineCacheUpdate be managed by PContent - v4, r=honzab

Thanks for Honza's feedback.
Rebase and carry reviewer's name.

Try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2cba8a409556

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/536546ad1bd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea30ee39b92

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/536546ad1bd9
https://hg.mozilla.org/mozilla-central/rev/cea30ee39b92