1260876 - Remove switch process code for signed packages

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Tanvi Vyas[:tanvi]]

Per Henry and Jonas, the code added in bug 1186290 is no longer needed: 
https://hg.mozilla.org/mozilla-central/rev/ca240e275047

With the patches in bug 1105556, netwerk/test/mochitests/test_signed_web_packaged_app.html is failing because TabParent::ShouldSwitchProcess depends on loadInfo->loadingPrincipal.  loadInfo->loadingPrincipal doesn't exist for TYPE_DOCUMENT loads with Bug 1105556.

Since this test and code is no longer needed, we need to remove it in order to land Bug 1105556.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f445d1751d5b&selectedJob=18732667

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I do want to note that we should keep the code which enables switching child processes on navigation. The logic for when we'll switch will likely change, but being able to switch is something that I think will be quite useful once e10s is more mature.

Comment 2

[Henry Chang [:hchang]]

(In reply to Jonas Sicking (:sicking) from comment #1)
> I do want to note that we should keep the code which enables switching child
> processes on navigation. The logic for when we'll switch will likely change,
> but being able to switch is something that I think will be quite useful once
> e10s is more mature.

The support of process switch is in nsFrameLoader::SwitchProcessAndLoadURI which will not be removed in this patch definitely :)

Comment 3

[Tanvi Vyas[:tanvi]]

I believe this is also causing the patches in Bug 1105556 to fail this origin attribute test:
netwerk/test/mochitests/test_origin_attributes_conversion.html

We end up without a loadingPrincipal here - http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#516 - and the test times out.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e45786f407ef&selectedJob=19014318

Comment 4

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1260876-Remove-process-switch-code-for-signed-pa.patch

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8738947 [details] [diff] [review]
0001-Bug-1260876-Remove-process-switch-code-for-signed-pa.patch

Hi Valentin,

Could you help review the patch to remove the process switch part and test cases for signed package? (since it's no longer needed.) I didn't remove all stuff about signing but only code which involved process switch. Thanks!

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8738947 [details] [diff] [review]
0001-Bug-1260876-Remove-process-switch-code-for-signed-pa.patch

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

Looks good. Thanks!

Comment 7

[Tanvi Vyas[:tanvi]]

Thanks Henry for your patch!  Can you push it to try?

Comment 8

[Henry Chang [:hchang]]

(In reply to Tanvi Vyas - behind on needinfos [:tanvi] from comment #7)
> Thanks Henry for your patch!  Can you push it to try?

I actually did it last night but it seems some warnings caused build error on certain platform :( I'll update the patch and land it as soon as possible. Thanks!

Comment 9

[Henry Chang [:hchang]]

Attachment: Patch (carried r+ from previous review)

Comment 10

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d4023e2c617

Waiting for try result.

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Henry Chang [:henry] from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d4023e2c617
> 
> Waiting for try result.

Looks like you are having a problem with netwerk/test/mochitests/test_origin_attributes_conversion.html.  That called SwitchProcess before you removed this code.  I was also having a problem with this test with my patches to bug 1105556.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

I think we can remove that part too, right?

Comment 13

[Henry Chang [:hchang]]

(In reply to Valentin Gosu [:valentin] from comment #12)
> I think we can remove that part too, right?

If it's just a origin attribute conversion, I feel like it should still work cuz it's supposed to have nothing to do with process switch. I will have a detailed look today on my desktop today. Thanks!

Comment 14

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #13)
> (In reply to Valentin Gosu [:valentin] from comment #12)
> > I think we can remove that part too, right?
> 
> If it's just a origin attribute conversion, I feel like it should still work
> cuz it's supposed to have nothing to do with process switch. I will have a
> detailed look today on my desktop today. Thanks!

After reading the test case, it is indeed needed to be removed :p Sorry for my misunderstanding in the previous comment.

Comment 15

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=335c3860ba82

One more try!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/465581fb7e66

Comment 17

[Henry Chang [:hchang]]

Try looks good so pushed it to inbound!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/465581fb7e66