Closed Bug 1020186 Opened 5 years ago Closed 5 years ago

Make POfflineCacheUpdate be managed by PContent

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: kanru, Assigned: kershaw)

References

Details

(Whiteboard: [nested-oop][ft:conndevices])

Attachments

(2 files, 4 obsolete files)

nsOfflineCacheUpdateService is chrome process only. Actually TabParent doesn't handle any offline cache related thing so it make sense to move it to PContent.
Whiteboard: [nested-oop]
feature-b2g: --- → 2.1
Whiteboard: [nested-oop] → [nested-oop][FT:Stream3]
Blocks: 1033984
Assignee: nobody → swu
Target Milestone: --- → 2.1 S3 (29aug)
Assignee: swu → kechang
Blocks: 1020157
No longer blocks: 1020157
Depends on: 1020157
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.
Flags: needinfo?(kchen)
(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.
Flags: needinfo?(kchen)
Attachment #8475747 - Flags: feedback?(kchen)
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 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?
Attachment #8475747 - Flags: feedback?(kchen) → feedback+
(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.
Flags: needinfo?(kchen)
(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.
Flags: needinfo?(kchen)
Hi Honza, Fabrice,

May I have your feedbacks about this patch?

Thanks.
Attachment #8475747 - Attachment is obsolete: true
Attachment #8477321 - Flags: review?(honzab.moz)
Attachment #8477321 - Flags: feedback?(fabrice)
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.
Attachment #8477321 - Flags: feedback?(fabrice)
I think Kan-Ru knows more specific details than me.
Kan-Ru, would you please help to answer Fabrice's question?
Flags: needinfo?(kchen)
(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..
Flags: needinfo?(kchen)
Flags: needinfo?(fabrice)
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.
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.
Flags: needinfo?(fabrice)
feature-b2g: 2.1 → 2.2
Target Milestone: 2.1 S3 (29aug) → ---
No longer blocks: 1033984
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Depends on: 1020172
Attachment #8477321 - Flags: review?(honzab.moz)
feature-b2g: 2.2? → ---
Whiteboard: [nested-oop][FT:Stream3] → [nested-oop][ft:conndevices]
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.
Attachment #8518012 - Flags: review?(khuey)
No longer depends on: 1020157
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.
Attachment #8477321 - Attachment is obsolete: true
Attachment #8518023 - Flags: review?(honzab.moz)
Comment on attachment 8518012 [details] [diff] [review]
Patch1: Revise the API in ContentProcessManager - v1

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

r=me
Attachment #8518012 - Flags: review?(khuey) → review+
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... );
Attachment #8518023 - Flags: review?(honzab.moz) → review+
Rebase and carry reviewer's name.
Attachment #8518012 - Attachment is obsolete: true
Attachment #8522077 - Flags: review+
Thanks for Honza's feedback.
Rebase and carry reviewer's name.

Try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2cba8a409556
Attachment #8518023 - Attachment is obsolete: true
Attachment #8522083 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/536546ad1bd9
https://hg.mozilla.org/mozilla-central/rev/cea30ee39b92
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.