Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Description • 10 years ago

Currently we only allow creating nested mozbrowser iframe. In case if we have an app and it's not a browser element we should still create a nested oop iframe for it.

	Kershaw Chang [:kershaw] Assignee
	Comment 1 • 10 years ago

Hi Kan-Ru,

Would you please help to review this patch?

Thanks.

	Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Comment 2 • 10 years ago

Comment on attachment 8461340 [details] [diff] [review]
Creating nested oop MozApp iframe

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

A good start but I think we need some refactoring.

Currently there are two functions to allocate a new process. One is GetNewOrUsed() for browser tab, one is MaybeTakePreallocatedAppProcess() for apps. I think we could rename them so it's obvious they are related. And call them in a CreateChildProcess() method that could be used in RecvCreateChildProcess() and the chrome process directly.

::: dom/ipc/ContentParent.cpp
@@ +805,2 @@
>  
> +    if (tc.IsValid()) {

How could you use this to distinguish browser or app?

@@ +820,5 @@
> +        tookPreallocated = !!cp;
> +        if (!tookPreallocated) {
> +            NS_WARNING("RecvCreateChildProcess: Unable to use pre-allocated app process");
> +            cp = new ContentParent(nullptr,
> +                             /* aOpener = */ nullptr,

aOpener = this;

@@ +950,5 @@
>          }
>          return nullptr;
>      }
>  
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {

Too many duplicated logic here for content process and below for chrome process..

	Kershaw Chang [:kershaw] Assignee
	Comment 3 • 10 years ago

Hi Kan-Ru,

Thanks for your useful comments. I've uploaded the v2 patch which includes the following changes:
1. Renaming MaybeTakePreallocatedAppProcess() and GetNewOrUsed() to GetNewOrUsedAppProcess() and GetNewOrUsedBrowserProcess() respectively.
2. Use GetOwnApp() to distinguish browser or app.
3. Remove some duplicate codes as much as possible.

Please help to review it. Thanks.

	Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Comment 4 • 10 years ago

Comment on attachment 8463287 [details] [diff] [review]
Creating nested oop MozApp iframe - v2

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

Cool, you are getting close.

Next time please upload patch with at least 8 lines of context. See https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

::: dom/ipc/ContentParent.cpp
@@ +585,5 @@
>  }
>  
>  /*static*/ already_AddRefed<ContentParent>
> +ContentParent::GetNewOrUsedAppProcess(const nsAString& aAppManifestURL,
> +                                      ProcessPriority aInitialPriority)

GetNewOrPreallocatedAppProcess

And this function currently only tries to get the preallocated process. You can move the |new ContentParent| part here so it actually tries to create a new process.

@@ +804,5 @@
> +    MaybeInvalidTabContext tc(aContext);
> +    if (!tc.IsValid()) {
> +        NS_ERROR(nsPrintfCString("Received an invalid TabContext from "
> +                             "the child process. (%s)",
> +                             tc.GetInvalidReason()).get());

nit: Align the arguments

@@ +830,5 @@
> +                                   /* isForPreallocated = */ false,
> +                                   aPriority);
> +            cp->Init();
> +        }
> +        sAppContentParents->Put(manifestURL, cp);

We don't want to reuse the app process in the nested case so no need to update hashtable.

@@ +880,5 @@
>  
>      ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
> +    bool isInContentProcess = (XRE_GetProcessType() != GeckoProcessType_Default) ?
> +                              true :
> +                              false;

bool isInContentProcess = (XRE_GetProcessType() != GeckoProcessType_Default);

@@ +1050,5 @@
> +        parent->IsForBrowser());
> +
> +    if (isInContentProcess) {
> +        return static_cast<TabParent*>(browser);
> +    }

Can't return here. The following check & retry is needed.

@@ +1076,5 @@
>          // Otherwise just give up.
>          return nullptr;
>      }
>  
> +    parent->AsContentParent()->MaybeTakeCPUWakeLock(aFrameElement);

This will conflict with bug 874353.

We should probably make sure the nested app process get the same priority boost. But that could be a followup.

Note to myself: we should check the expecting-system-message from chrome process and not rely on the expecting-system-message attribute.

::: dom/ipc/ContentParent.h
@@ +98,5 @@
>  
>      static already_AddRefed<ContentParent>
> +    GetNewOrUsedBrowserProcess(bool aForBrowserElement = false,
> +                               hal::ProcessPriority aPriority =
> +                                hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,

nit: indent

	Kershaw Chang [:kershaw] Assignee
	Comment 5 • 10 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #4)
> Comment on attachment 8463287 [details] [diff] [review]
> Creating nested oop MozApp iframe - v2
> 
> Review of attachment 8463287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, you are getting close.
> 
> Next time please upload patch with at least 8 lines of context. See
> https://developer.mozilla.org/en-US/docs/
> Creating_a_patch_that_can_be_checked_in
> 
> ::: dom/ipc/ContentParent.cpp
> @@ +585,5 @@
> >  }
> >  
> >  /*static*/ already_AddRefed<ContentParent>
> > +ContentParent::GetNewOrUsedAppProcess(const nsAString& aAppManifestURL,
> > +                                      ProcessPriority aInitialPriority)
> 
> GetNewOrPreallocatedAppProcess
> 
> And this function currently only tries to get the preallocated process. You
> can move the |new ContentParent| part here so it actually tries to create a
> new process.
> 
> @@ +804,5 @@
> > +    MaybeInvalidTabContext tc(aContext);
> > +    if (!tc.IsValid()) {
> > +        NS_ERROR(nsPrintfCString("Received an invalid TabContext from "
> > +                             "the child process. (%s)",
> > +                             tc.GetInvalidReason()).get());
> 
> nit: Align the arguments
> 
> @@ +830,5 @@
> > +                                   /* isForPreallocated = */ false,
> > +                                   aPriority);
> > +            cp->Init();
> > +        }
> > +        sAppContentParents->Put(manifestURL, cp);
> 
> We don't want to reuse the app process in the nested case so no need to
> update hashtable.
> 
> @@ +880,5 @@
> >  
> >      ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
> > +    bool isInContentProcess = (XRE_GetProcessType() != GeckoProcessType_Default) ?
> > +                              true :
> > +                              false;
> 
> bool isInContentProcess = (XRE_GetProcessType() != GeckoProcessType_Default);
> 
> @@ +1050,5 @@
> > +        parent->IsForBrowser());
> > +
> > +    if (isInContentProcess) {
> > +        return static_cast<TabParent*>(browser);
> > +    }
> 
> Can't return here. The following check & retry is needed.
The following check is only for ContentParent, not for ContentBridgeParent. Perhaps we should do this in chrome process, not in content process?

BTW, is it possible that SendPBrowserConstructor returns a nullptr in nested oop case?

Thanks.
> 
> @@ +1076,5 @@
> >          // Otherwise just give up.
> >          return nullptr;
> >      }
> >  
> > +    parent->AsContentParent()->MaybeTakeCPUWakeLock(aFrameElement);
> 
> This will conflict with bug 874353.
> 
> We should probably make sure the nested app process get the same priority
> boost. But that could be a followup.
> 
> Note to myself: we should check the expecting-system-message from chrome
> process and not rely on the expecting-system-message attribute.
> 
> ::: dom/ipc/ContentParent.h
> @@ +98,5 @@
> >  
> >      static already_AddRefed<ContentParent>
> > +    GetNewOrUsedBrowserProcess(bool aForBrowserElement = false,
> > +                               hal::ProcessPriority aPriority =
> > +                                hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
> 
> nit: indent

	Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Comment 6 • 10 years ago

(In reply to Kershaw Chang [:kershaw] from comment #5)
> > Can't return here. The following check & retry is needed.
> The following check is only for ContentParent, not for ContentBridgeParent.
> Perhaps we should do this in chrome process, not in content process?

Why is it not? There is nothing special for ContentBridgeParent

> BTW, is it possible that SendPBrowserConstructor returns a nullptr in nested
> oop case?

It is possible.

	Kershaw Chang [:kershaw] Assignee
	Comment 7 • 10 years ago

Hi Kan-Ru,

I've updated the patch as your feedback and our discussion result.
Please kindly help to review it.

Thanks.

	Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Comment 8 • 10 years ago

Comment on attachment 8466082 [details] [diff] [review]
Allow creating nested oop MozApp iframe - v3

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

Looks good. You forgot to rename the GetNewOrUsedAppProcess.

Kyle, could you help review the process creation changes?

::: dom/ipc/ContentParent.cpp
@@ +589,5 @@
>  /*static*/ already_AddRefed<ContentParent>
> +ContentParent::GetNewOrUsedAppProcess(mozIApplication* aApp,
> +                                      ProcessPriority aInitialPriority,
> +                                      ContentParent* aOpener,
> +                                      /*out*/ bool* aTookPreAllocated)

GetNewOrPreallocatedProcess and other places.

@@ +843,5 @@
> +                                    aPriority,
> +                                    this);
> +        if (!cp) {
> +            return false;
> +        }

Move this check after the if {} else {}

	Jenny Liu[:jennyliu]
	Comment 9 • 10 years ago

Move to 2.1 Sprint 2.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 10 • 10 years ago

Comment on attachment 8466082 [details] [diff] [review]
Allow creating nested oop MozApp iframe - v3

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

::: dom/ipc/ContentParent.h
@@ +100,5 @@
>      static already_AddRefed<ContentParent>
> +    GetNewOrUsedBrowserProcess(bool aForBrowserElement = false,
> +                               hal::ProcessPriority aPriority =
> +                               hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
> +                               ContentParent* aOpener = nullptr);

Why would we have a browser process for something other than a browser element?

	Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Comment 11 • 10 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Comment on attachment 8466082 [details] [diff] [review]
> Allow creating nested oop MozApp iframe - v3
> 
> Review of attachment 8466082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentParent.h
> @@ +100,5 @@
> >      static already_AddRefed<ContentParent>
> > +    GetNewOrUsedBrowserProcess(bool aForBrowserElement = false,
> > +                               hal::ProcessPriority aPriority =
> > +                               hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
> > +                               ContentParent* aOpener = nullptr);
> 
> Why would we have a browser process for something other than a browser
> element?

This method is also used create remote xul <browser> and maybe normal iframe in the future. Could you suggest a better name?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 12 • 10 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #11)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> > Comment on attachment 8466082 [details] [diff] [review]
> > Allow creating nested oop MozApp iframe - v3
> > 
> > Review of attachment 8466082 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/ipc/ContentParent.h
> > @@ +100,5 @@
> > >      static already_AddRefed<ContentParent>
> > > +    GetNewOrUsedBrowserProcess(bool aForBrowserElement = false,
> > > +                               hal::ProcessPriority aPriority =
> > > +                               hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
> > > +                               ContentParent* aOpener = nullptr);
> > 
> > Why would we have a browser process for something other than a browser
> > element?
> 
> This method is also used create remote xul <browser> and maybe normal iframe
> in the future. Could you suggest a better name?

GetNewOrUsedContentProcess?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 13 • 10 years ago

Comment on attachment 8466082 [details] [diff] [review]
Allow creating nested oop MozApp iframe - v3

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

I assume kanru has already reviewed the stuff in CreateBrowserOrApp.

::: dom/ipc/ContentParent.cpp
@@ +598,5 @@
> +    if (process) {
> +        if (!process->SetPriorityAndCheckIsAlive(aInitialPriority)) {
> +            // Kill the process just in case it's not actually dead; we don't want
> +            // to "leak" this process!
> +            process->KillHard();

Not returning after this is a behavior change, but it's a sensible one.

@@ +616,4 @@
>      }
>  
> +    // XXXkhuey Nuwa wants the frame loader to try again later, but the
> +    // frame loader is really not set up to do that ...

I should file a bug on this ...

::: dom/ipc/ContentParent.h
@@ +100,5 @@
>      static already_AddRefed<ContentParent>
> +    GetNewOrUsedBrowserProcess(bool aForBrowserElement = false,
> +                               hal::ProcessPriority aPriority =
> +                               hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
> +                               ContentParent* aOpener = nullptr);

Now that I've thought about this more I think this name is fine.  Please add a comment about what it is used for.

@@ +291,5 @@
>      static already_AddRefed<ContentParent>
> +    GetNewOrUsedAppProcess(mozIApplication* aApp,
> +                           hal::ProcessPriority aInitialPriority,
> +                           ContentParent* aOpener,
> +                           /*out*/ bool* aTookPreAllocated = nullptr);

GetNewOrPreallocatedAppProcess.

	Kershaw Chang [:kershaw] Assignee
	Comment 14 • 10 years ago

Update the patch with the following changes:
1. rebase to the master again
2. add some small changes mentioned above
3. carry reviewer's name

	Kershaw Chang [:kershaw] Assignee
	Comment 15 • 10 years ago

try: https://tbpl.mozilla.org/?tree=Try&rev=cd2bb4cfcd4e

Looks OK.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/15eb43f20248

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/15eb43f20248