Closed Bug 1216443 Opened 5 years ago Closed 5 years ago

Navigating from "about:blank" to signed packaged web content doesn't trigger process switch.

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(1 file, 3 obsolete files)

In new security model, we require the signed packaged content to be loaded in its own process. We use a couple of condition [1] to determine whether we need to reload the content in a separate process. The way we use to tell if a process is the newly created one is to check if its origin is "about:blank". However, in Bug 1180088, we rely on "switching process" to assign the "signed package origin" to the TabContext, which means we hope "process switch" would still happen even we navigate from "about:blank".

My gut feeling is to check if the TabParent has empty mSignedPkgOriginNoSuffix, which is going to be added by Bug 1180088. If it's empty, a process switch is required.

[1] http://hg.mozilla.org/mozilla-central/file/9605da94e75d/dom/ipc/TabParent.cpp#l467
Assignee: nobody → hchang
Attachment #8701921 - Attachment is obsolete: true
Attachment #8705492 - Attachment is obsolete: true
Comment on attachment 8705493 [details] [diff] [review]
0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch

Hi Kanru,

Could you help review this patch? It changes the condition whether we need to switch process when navigating from moz-safe-about:blank. We no longer whitelist the origins to decide if we should switch process. Instead, we check if this tab is created for loading a signed page.
Attachment #8705493 - Flags: review?(kchen)
Comment on attachment 8705493 [details] [diff] [review]
0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch

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

From the test case you seem to expect 2 processes to be created. IMO we should create only process in this case because the iframe is brand-new. It's no point to create one new prosess then immediately switch to another one. If you want to address this in a new bug, please request review again.
Attachment #8705493 - Flags: review?(kchen) → review-
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #5)
> Comment on attachment 8705493 [details] [diff] [review]
> 0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch
> 
> Review of attachment 8705493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> From the test case you seem to expect 2 processes to be created. IMO we
> should create only process in this case because the iframe is brand-new.
> It's no point to create one new prosess then immediately switch to another
> one. If you want to address this in a new bug, please request review again.

Well, this is unfortunately what it is supposed to be to me :(

The reason is we heavily rely on TabContext.OriginAttributes to make decisions like permission and the need of process switch. For a brand-new process which is not created by nsFrameLoader::SwitchProcessAndLoadURL, its TabContext is brand-new as well. Since the TabContext should be immutable (could we change this?) after creation, we have to do things like this.

Two things we must do to get rid of this duplicate process switch:

1) Re-design parent process permission check (i.e. [1])
2) Use loadInfo instead of loadContext to decide if we need to switch process since we currently rely on the docShell.originAttributes for process switch.

I would think (2) is gonna be fixed after having a better solution to Bug 1178526 (actually I have a dev branch to deal with this [2]). However, (1) is pretty tough to me ...

So, do you think we should just file a bug and leave it open up for discussion?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#154
[2] https://github.com/elefant/gecko-dev/commits/Better-Bug1178526
Flags: needinfo?(kchen)
(In reply to Henry Chang [:henry] from comment #6)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #5)
> > Comment on attachment 8705493 [details] [diff] [review]
> > 0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch
> > 
> > Review of attachment 8705493 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > From the test case you seem to expect 2 processes to be created. IMO we
> > should create only process in this case because the iframe is brand-new.
> > It's no point to create one new prosess then immediately switch to another
> > one. If you want to address this in a new bug, please request review again.
> 
> Well, this is unfortunately what it is supposed to be to me :(
> 
> The reason is we heavily rely on TabContext.OriginAttributes to make
> decisions like permission and the need of process switch. For a brand-new
> process which is not created by nsFrameLoader::SwitchProcessAndLoadURL, its
> TabContext is brand-new as well. Since the TabContext should be immutable
> (could we change this?) after creation, we have to do things like this.

But this is the TabChild's first load. I think we might be ablo to delay TabChild::NotifyTabContextUpdated() until we know it's a new signed package url or not, can we? Maybe not, because we decides it asynchronously.

> Two things we must do to get rid of this duplicate process switch:
> 
> 1) Re-design parent process permission check (i.e. [1])
> 2) Use loadInfo instead of loadContext to decide if we need to switch
> process since we currently rely on the docShell.originAttributes for process
> switch.
> 
> I would think (2) is gonna be fixed after having a better solution to Bug
> 1178526 (actually I have a dev branch to deal with this [2]). However, (1)
> is pretty tough to me ...
> 
> So, do you think we should just file a bug and leave it open up for
> discussion?

Yeah, maybe.

> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.
> cpp#154
> [2] https://github.com/elefant/gecko-dev/commits/Better-Bug1178526
Flags: needinfo?(kchen)
Comment on attachment 8708908 [details] [diff] [review]
0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch

Hi Kanru,

I attached another patch which is annotated a filed bug 1239902 for the redundant process issue. Could you please review the patch again? Thanks!
Attachment #8708908 - Flags: review?(kchen)
Attachment #8708908 - Flags: review?(kchen) → review+
Keywords: checkin-needed
Thanks Kanru!
https://hg.mozilla.org/mozilla-central/rev/67c94e29bcac
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.