Closed Bug 1260876 Opened 4 years ago Closed 4 years ago

Remove switch process code for signed packages

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: tanvi, Assigned: hchang)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

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
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.
(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 :)
Assignee: nobody → hchang
Whiteboard: [necko-active]
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 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!
Attachment #8738947 - Flags: review?(valentin.gosu)
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!
Attachment #8738947 - Flags: review?(valentin.gosu) → review+
Thanks Henry for your patch!  Can you push it to try?
(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!
Attachment #8738947 - Attachment is obsolete: true
Attachment #8739366 - Flags: review+
(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.
I think we can remove that part too, right?
(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!
(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.
Try looks good so pushed it to inbound!
https://hg.mozilla.org/mozilla-central/rev/465581fb7e66
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
See Also: → 1262590
You need to log in before you can comment on or make changes to this bug.