Closed Bug 1020157 Opened 6 years ago Closed 5 years ago

NeckoParent::GetValidatedAppInfo should recognize nested oop iframe

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
2.2 S2 (19dec)

People

(Reporter: kanru, Assigned: kershaw)

References

Details

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

Attachments

(1 file, 5 obsolete files)

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.
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?
Flags: needinfo?(kchen)
(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.
Flags: needinfo?(kchen)
Assignee: nobody → kchen
feature-b2g: --- → 2.1
Whiteboard: [nested-oop]
Blocks: 1033984
Whiteboard: [nested-oop] → [nested-oop][FT:Stream3]
Attachment #8452302 - Flags: review?(khuey)
Attachment #8452306 - Flags: review?(jduell.mcbugs)
(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.
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.
Attachment #8452306 - Flags: review?(jduell.mcbugs)
Target Milestone: --- → 2.1 S1 (1aug)
Blocks: 1020172
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 ..."?
Attachment #8452304 - Flags: review?(khuey) → review+
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 :)
Attachment #8452306 - Attachment is obsolete: true
Attachment #8467637 - Flags: review?(jduell.mcbugs)
Move to 2.1 Sprint 2
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
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.
Attachment #8467637 - Flags: review?(khuey)
Attachment #8467637 - Flags: review?(jduell.mcbugs)
Attachment #8467637 - Flags: feedback+
Depends on: 1029352
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
Move to v2.1 sprint 3.
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Depends on: 1020186
Blocks: 1020186
No longer depends on: 1020186
Blocks: 1020196
Depends on: 1057230
No longer blocks: 1033984
feature-b2g: 2.1 → 2.2
Target Milestone: 2.1 S3 (29aug) → ---
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Depends on: 120172
No longer blocks: 1020172
Depends on: 1020172
No longer depends on: 120172
Blocks: 1057230
No longer depends on: 1057230
Assignee: kchen → kechang
feature-b2g: 2.2? → ---
Whiteboard: [nested-oop][FT:Stream3] → [nested-oop][ft:conndevices]
Rebase since bug 1020172 is landed.

Hi Jason,

Could you look at this patch?

Thanks.
Attachment #8452302 - Attachment is obsolete: true
Attachment #8452304 - Attachment is obsolete: true
Attachment #8467637 - Attachment is obsolete: true
Attachment #8517436 - Flags: review?(jduell.mcbugs)
No longer blocks: 1020186
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.
Flags: needinfo?(jduell.mcbugs)
Target Milestone: --- → 2.2 S1 (5dec)
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?
Attachment #8517436 - Flags: review?(jduell.mcbugs) → review+
(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.
Flags: needinfo?(jduell.mcbugs)
https://hg.mozilla.org/mozilla-central/rev/38756d1a5d2b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: 2.2 S1 (5dec) → 2.2 S2 (19dec)
You need to log in before you can comment on or make changes to this bug.