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

Currently nested oop iframe is killed when trying to access network because the PContentParent doesn't manage any PBrowserParent.

We should pass the embedder iframe's PContentParent to GetValidatedAppInfo or make it recognize embedded app.

	Jason Duell
	Comment 1 • 10 years ago

What are we trying to do that's causing this to happen?  Some context would be useful here.

I'm not clear on the fix here--right now the PContentParent that gets passed to GetValidatedAppInfo is one we get by calling Manager(). Do we need to get a different ContentParent, or somehow arrange for the one we're getting now to manage (or otherwise point to) the right PBrowserParent?

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

(In reply to Jason Duell (:jduell) from comment #1)
> What are we trying to do that's causing this to happen?  Some context would
> be useful here.
> 
> I'm not clear on the fix here--right now the PContentParent that gets passed
> to GetValidatedAppInfo is one we get by calling Manager(). Do we need to get
> a different ContentParent, or somehow arrange for the one we're getting now
> to manage (or otherwise point to) the right PBrowserParent?

This is filed to track the remaining issues of nested oop iframe after we land bug 879475.

Chrome Process           Other Processes
+- PContentParent[A] --- PContentChild
|  `-- PBrowesParent ---- PBrowserChild
|                        PContentBridgeParent
|                        `--- PBrowserParent --+
|                                              |
`- PContentParent[B] --- PContentChild         |
   PNecko                PContentBridgeChild   |
                         `--- PBrowserChild[C]-+

It happens when a nested PBrowser[C] managed by PContentBridge opens a new channel via PContent[B]. The PContentParent[B] doesn't manage the PBrowserParent in the other process directly.

So I think we would have a (PContentParent, PBrowserId) => AppInfo map in the chrome process so we could get the app info in such case.

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

(In reply to Kan-Ru Chen [:kanru] from comment #4)
> Created attachment 8452304 [details] [diff] [review]
> Part 2. Expose OwnOrContainingAppId from ContentParent

Note this doesn't work for nested mozapp + Nuwa preallocated process yet since Nuwa doesn't use the mOpener parameter. This will be fixed in bug 1020204.

	Jason Duell
	Comment 7 • 10 years ago

Comment on attachment 8452306 [details] [diff] [review]
Part 3. Get AppId from ContentParent

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

So this patch is changing the same code as bug 1029352.  Can you 1) make sure that patch doesn't fix this, and 2) if not, rebase this patch on top of that one?  Thanks.

I'm going to be on PTO for close to a week--if I'm not available to review I'm fine with bent or khuey reviewing this.  They probably understand the magic here better than I do anyway :)

::: netwerk/ipc/NeckoParent.cpp
@@ +110,5 @@
>  const char*
>  NeckoParent::GetValidatedAppInfo(const SerializedLoadContext& aSerialized,
>                                   PContentParent* aContent,
> +                                 /* out */ uint32_t* aAppId,
> +                                 /* out */ bool* aInBrowserElement)

We don't generally mark "out" parameters with comments like this.  Not a bad idea but not done anywhere else in this part of the tree, so I'd remove just for style consistency.

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

Comment on attachment 8452304 [details] [diff] [review]
Part 2. Expose OwnOrContainingAppId from ContentParent

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

::: dom/ipc/ContentParent.h
@@ +643,5 @@
>       */
>      nsString mAppName;
>  
> +    uint32_t mOwnAppId;
> +    uint32_t mContainingAppId;

Delete the \n above mOwnAppId and change the comment to say "We cache these values instead ..."?

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

Rebased on bug 1029352

Bug 1029352 doesn't affect this bug. It removed a direct TabParent usage in AllocPRemoteOpenFileParent which is nice to base on :)

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

Move to 2.1 Sprint 2

	Jason Duell
	Comment 11 • 10 years ago

Comment on attachment 8467637 [details] [diff] [review]
Part 3. Get AppId from ContentParent

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

Looks OK to me but I think bent or khuey should sign off too.

::: netwerk/ipc/NeckoParent.cpp
@@ +104,5 @@
> +                                 "dom.ipc.reuse_parent_app", false);
> +  }
> +  return sCanReuseParentApp;
> +}
> +} // anonymous namespace

We generally avoid anonymous namespaces? I'd just make it static.

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

Hi Kyle,

Sorry for bothering.
Since we're trying to land it by the end of this week or early next week, could you please help to take a look and provide your suggestion?
Thanks a lot!!! :D

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

Move to v2.1 sprint 3.

	Kai-Chih Hu [:khu]
	Comment 14 • 10 years ago

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

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

Rebase since bug 1020172 is landed.

Hi Jason,

Could you look at this patch?

Thanks.

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

Hi Jason,

Sorry for bothering. Since it's almost two weeks without your feedback, could you spend some time to look at this patch?
I believe this patch is quite easy and won't take too much of your time.

Thanks.

	Jason Duell
	Comment 17 • 10 years ago

Comment on attachment 8517436 [details] [diff] [review]
Replace ManagedPBrowserParent with GetManagedTabContext in NeckoParent

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

Looks good. I assume we test this codepath somewhere?

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

(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #17)
> Comment on attachment 8517436 [details] [diff] [review]
> Replace ManagedPBrowserParent with GetManagedTabContext in NeckoParent
> 
> Review of attachment 8517436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I assume we test this codepath somewhere?

This codepath should be exercised by the e10s tests. So, yes.

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

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

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

Carry reviewer's name.

	Carsten Book [:Tomcat]
	Comment 21 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/38756d1a5d2b

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

https://hg.mozilla.org/mozilla-central/rev/38756d1a5d2b