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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Henry Chang [:hchang]]

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

Comment 1

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch

Comment 2

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch

Comment 3

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch

Comment 4

[Henry Chang [:hchang]]

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.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 6

[Henry Chang [:hchang]]

(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

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(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

Comment 8

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1216443-moz-safe-about-blank-to-signed-content-s.patch

Comment 9

[Henry Chang [:hchang]]

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!

Comment 10

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19038bc6f941

Comment 11

[Henry Chang [:hchang]]

Thanks Kanru!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67c94e29bcac

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67c94e29bcac