1355166 - Remove remote newtab's dead code

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Ursula Sarracini (:ursula)]

Files found in browser/components/newtab used to belong to the remote newtab project. Since this died we should get rid of the dead code. 
One approach is what is mentioned in bug 1329955

Comment 1

[Ed Lee :Mardak]

As noted in bug 1360316, there was some problems caused by relying on dead code as NewTabPrefsProvider is no longer being `init`ed.

Comment 2

[Tim Nguyen :ntim]

See also: https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/browser/base/content/test/static/browser_all_files_referenced.js#93

Comment 3

[Adam Kasztenny]

I'd like to work on this.

Comment 4

[Ed Lee :Mardak]

Is there a good way to just remove the "remote newtab" stuff without impacting tiles and activity stream? In particular, both depend on aboutNewTabService but probably not everything in browser/components/newtab

https://searchfox.org/mozilla-central/search?q=aboutNewTabService

Any good way for Adam to test if things get broken or not when cleaning up?

Comment 5

[Ursula Sarracini (:ursula)]

Hey Adam, thanks for your interest here. 

You can delete everything in browser/components/newtab EXCEPT for aboutNewTabService, also delete https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js#93-95, and also delete test_NewTabPrefsProvider.js and test_NewTabSearchProvider.js in browser/components/newtab/tests/xpcshell - that means removing them from xpcshell.ini as well. 

As a first step, make sure to run the tests in browser/components/newtab/tests to make sure they're still good. If you want to make sure tiles code isn't busted you can run the tests in browser/base/content/test/newtab and make sure they still pass. But really, running it on try is your best bet to make sure nothing is broken. If you don't have try privileges, I can apply your patch and run the test suite to double check. The remote newtab code should be pretty isolated, so nothing should be breaking the new stuff when removed, and as long as you remove all the stuff mentioned above, you should be ok.

There's also some content signing stuff that's left over in https://dxr.mozilla.org/mozilla-central/source/dom/security/ContentVerifier.cpp#51, and tests associated with it: https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_content_signing.js#15. This should also be dead code

Comment 6

[Adam Kasztenny]

Thanks a lot for the information. Does it make sense to run the entire test suite locally before submitting the patch? Or does that take too long? (I'm new here, and I haven't tried running any tests yet)

Comment 7

[Ursula Sarracini (:ursula)]

I wouldn't run the entire test suite locally, that would take much too long! The great thing about try is that you can run it on multiple platforms. I think starting with the tests that I mentioned above is good, and then I can push it to try and see if there's anything else broken :)

Have you run tests locally before?

Comment 8

[Adam Kasztenny]

Okay, thanks, I'll do that then. I haven't tried running any tests locally yet.

Comment 9

[Ursula Sarracini (:ursula)]

Here are some docs on running them locally to get you started: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Running_automated_tests
You'll mostly be interested in xpcshell tests: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests and mochitests: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest

Comment 10

[Adam Kasztenny]

I was able to run tests fine. Everything passed before I started deleting things, and afterwards as well. I ended up deleting:

browser/components/newtab/NewTabPrefsProvider.jsm
browser/components/newtab/NewTabRemoteResources.jsm
browser/components/newtab/NewTabSearchProvider.jsm
browser/components/newtab/NewTabWebChannel.jsm
browser/components/newtab/tests/xpcshell/test_NewTabPrefsProvider.js
browser/components/newtab/tests/xpcshell/test_NewTabSearchProvider.js

along with modifying moz.build and xpcshell.ini.

However, I'm not sure what to delete from https://dxr.mozilla.org/mozilla-central/source/dom/security/ContentVerifier.cpp (it seems to be used, along with its init method which references newtab) and from the tests, https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_content_signing.js (should I delete every test that makes reference to newtab?).

When I'm done, how should I submit my patch? Should I post it here or email it to you?

Thanks.

Comment 11

[Ursula Sarracini (:ursula)]

Hey Franziskus, Adam is working on removing some of the leftover dead code for remote newtab, some code was added for Bug 1263793 that can probably be deleted now, including some tests in dom/security/test/contentverifier/. Would you be able to provide Adam some help on what parts can be deleted in ContentVerifier.cpp and which tests to make sure are ok?

If it's out of scope for this patch, or a first time contributor, then maybe we can just stick to removing the dead code in browser/components/newtab and file another bug to remove the content signing bits later? What do you think Franziskus?

Adam, when you're finished with the patch, submit it as an attachment to this Bug so we can review it and land it for you.

Comment 12

[Franziskus Kiefer [:franziskus]]

> some code was added for Bug 1263793 that can probably be deleted now, including some tests in dom/security/test/contentverifier/

The tests can definitely be deleted. They are disable anyway.

The only two services (I know of) using content signatures right now are blocklist updates and normandy. They both use the CS service directly. So I think ContentVerifier.cpp can go completely. To make that work there are changes to nsHTTPChannel necessary.
However, before doing that it might be advisable to check whether this is used anywhere else. I don't think it is and a quick check doesn't reveal any callers of LoadInfo::SetVerifySignedContent (and without it content signature verification is not invoked from nsHTTPChannel). The LoadInfo bits can be removed as well then as it can't really be used anymore.
This is touching a couple different parts but I think it's all dead code right now and should probably be removed.

Comment 13

[Adam Kasztenny]

Thanks. nsHTTPChannel seems to be still used. What file/function are you referring to when you say "whether this is used anywhere else"?

Comment 14

[Adam Kasztenny]

Attachment: Deleting js and jsm files that were dead

This patch doesn't include any of the cpp changes.

Comment 15

[Franziskus Kiefer [:franziskus]]

> nsHTTPChannel seems to be still used. 

nsHTTPChannel is our HTTP channel so that won't go away ;) But it calls functions from ContentVerifier.cpp. So you can't remove ContentVerifier.cpp without touching nsHTTPChannel.

> What file/function are you referring to when you say "whether this is used anywhere else"?

Any public function from ContentVerifier. If it's used in a context that's not protected by the SetVerifySignedContent LoadInfo flag, or the SetVerifySignedContent LoadInfo flag is set anywhere, then it's used. (I don't think that's the case but you should probably double check.)

Comment 16

[Adam Kasztenny]

Thanks. So I should remove the unused functions from ContentVerifier?
Ursula, could you please check if any tests are broken as a result of my JS changes in the patch?

Comment 17

[Ursula Sarracini (:ursula)]

Hey Adam, can you rebase this patch on the latest mozilla-central? I'm not getting a clean apply

Comment 18

[Adam Kasztenny]

Yeah, I'll do that.

Comment 19

[Adam Kasztenny]

Attachment: Deleting js and jsm files that were dead, after rebasing

Comment 20

[Adam Kasztenny]

Okay, the patch should be ready to go now.

Comment 21

[Ursula Sarracini (:ursula)]

Try push for the most recent patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb7cc4c93092b7391daf63e3f5ef3e9d7e8b2c2f

If this is green then I'll start the review. Thanks for the rebase Adam!

Comment 22

[Ursula Sarracini (:ursula)]

Adam, you'll need to delete these lines of code: https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/browser/base/content/test/static/browser_all_files_referenced.js#93-95

That will fix some of the initial test failures from the try push.

Comment 23

[Adam Kasztenny]

Attachment: The rebased patch with the fix

Comment 24

[Ursula Sarracini (:ursula)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59b817676db41111b3e48568b9689018881132fa

Comment 25

[Adam Kasztenny]

I see 3 test failures. Are these all unrelated to my changes?

Comment 26

[Ursula Sarracini (:ursula)]

They don't look related to your changes. I'll have a review for you done today :)

Comment 27

[Adam Kasztenny]

Excellent, thanks!

Comment 28

[Ursula Sarracini (:ursula)]

I think you can remove the remaining file dom/security/test/contentverifier/file_contentserver.sjs. So you can remove the entire folder

Comment 29

[Adam Kasztenny]

Attachment: Bug 1355166 - Remove remote newtab's dead code r=ursula

Comment 30

[Ursula Sarracini (:ursula)]

Comment on attachment 8953296 [details] [diff] [review]
Bug 1355166 - Remove remote newtab's dead code r=ursula

This looks good to me. We can properly remove ContentVerifier.cpp as a follow up bug, but this is a good start. Thanks!

Comment 31

[Ursula Sarracini (:ursula)]

Can you change the commit message to be:
"Bug 1355166 - Remove remote newtab's dead code r=ursula"
And we can set check-in needed to land it

Comment 32

[Adam Kasztenny]

Thanks, I changed the name of the patch. I'll start on the CPP changes.

Comment 33

[Ursula Sarracini (:ursula)]

If you want to make the patch for the cpp changes on this bug, make sure to attach the patch as a separate attachment from the JS changes, and probably ask Franziskus for review on that one.

Comment 34

[Adam Kasztenny]

Yes, I'll be sure to do that.

Comment 35

[Adam Kasztenny]

A few files that I deleted were just changed today:

browser/components/newtab/NewTabPrefsProvider.jsm
browser/components/newtab/NewTabRemoteResources.jsm                                                                                                                  
browser/components/newtab/NewTabSearchProvider.jsm                                                                                                                    
browser/components/newtab/NewTabWebChannel.jsm

Comment 36

[Ed Lee :Mardak]

Looks like bug 1440284. Should still be able to just delete them.

Comment 37

[Adam Kasztenny]

Should the status of this bug be changed to "check-in needed"?

Comment 38

[Ursula Sarracini (:ursula)]

Well that depends, if you want to file the c++ changes in a different bug then yes I can set check-in needed, but I thought you were planning on attaching the changes to this bug? If so we have to wait for all the changes to be ready to land, then change the bug status so both patches can land

Comment 39

[Adam Kasztenny]

Oh okay, I thought it could be done in parts. I'll leave it up to you whether the C++ changes should be done in a different bug or this one. I'm fine either way.

Comment 40

[Ursula Sarracini (:ursula)]

Let's just do them in a separate bug so this can land. I can file it. Thanks!

Comment 41

[Adam Kasztenny]

Thank you!

Comment 42

[Ursula Sarracini (:ursula)]

Filed bug 1441989 for deleting ContentVerifier.cpp. And assigned Adam

Comment 43

[Cosmin Sabou [:CosminS]]

This patch fails to apply. Please take a look at it. Thanks!

applying 1355166-with-fix-2.patch
patching file browser/components/newtab/NewTabPrefsProvider.jsm
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/newtab/NewTabPrefsProvider.jsm.rej
patching file browser/components/newtab/NewTabRemoteResources.jsm
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/newtab/NewTabRemoteResources.jsm.rej
patching file browser/components/newtab/NewTabSearchProvider.jsm
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/newtab/NewTabSearchProvider.jsm.rej
patching file browser/components/newtab/NewTabWebChannel.jsm
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/newtab/NewTabWebChannel.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1355166-with-fix-2.patch

Comment 44

[Ed Lee :Mardak]

Pretty sure the patch needs to be rebased as per comment 36

Comment 45

[Adam Kasztenny]

Yep, thanks. I'll do that.

Comment 46

[Adam Kasztenny]

Attachment: Bug 1355166 - Remove remote newtab's dead code r=ursula

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42c835f417b5
Remove remote newtab's dead code. r=ursula

Comment 48

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/42c835f417b5