886907 - (killSyncContentPrefs) Remove old synchronous contentPrefService from the tree

Status: RESOLVED FIXED

(Toolkit :: General, defect, P1)

Description

[Marco Bonardo [:mak]]

a couple versions after the deprecation in bug 850219 we should remove the old synchronous service and, if possible, merge the new service module into the service. As reported in the module:

17 // The plan is to eventually remove nsIContentPrefService and its
18 // implementation, nsContentPrefService.js.  At such time this file can stop
19 // being a JSM, and the "_cps" parts that ContentPrefService2 relies on and
20 // NSGetFactory and all the other XPCOM initialization goop in
21 // nsContentPrefService.js can be moved here.

Comment 1

[Marco Bonardo [:mak]]

note also Services.contentPrefs, I'm not sure if we should just remove it, replace it, or introduce a contentPrefs2. It is only used in a couple places in our codebase, so maybe it's not worth to have it in Services.

Comment 2

[Marco Bonardo [:mak]]

This should also be good to kill the nsContentPrefService.js compartment at startup

Comment 3

[Marco Bonardo [:mak]]

Drew, would you be interested in mentoring the remaining work here?

Comment 4

[Drew Willcoxon :adw]

Sure!

Comment 5

[Marco Bonardo [:mak]]

I wonder if we can call this a "first" bug...

Comment 6

[Marco Bonardo [:mak]]

Drew, is this something easy enough to make it a [good first bug]?

Comment 7

[Drew Willcoxon :adw]

Sure, probably. :-)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Raj contacted me through email saying he'd like to work on this bug. I've assigned him.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Hey Raj,

Do you have a build of Firefox on your machine ready to go? If not, we can help you through that step.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

I've been communicating with Raj over email, and is getting his build set up.

In the meantime, adw, did you have some starting tips for Raj?

Comment 11

[Drew Willcoxon :adw]

There's kind of a lot of stuff to do, but fortunately it's pretty straightforward I think.  The steps are below.  And I'm probably forgetting things.  Let me know when you run into problems or need more details.

1. Modify ContentPrefService2.jsm

This file implements nsIContentPrefService2.  There's a separate file, nsContentPrefService.js, that implements the old service we're removing.  There's overlap in the functionality of the old and new service, so currently, to avoid reimplementing that overlap, ContentPrefService2.jsm relies on some implementation bits in nsContentPrefService.js.  We need to copy those bits into ContentPrefService2.jsm so that nsContentPrefService.js can be removed.  Please read the big comment at the top of ContentPrefService2.jsm for some info on that.

Sub-steps:

* Rename ContentPrefService2.jsm to ContentPrefService2.js (take off the "m" at the end): https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm

* Remove the big comment starting here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#5

* Remove EXPORTED_SYMBOLS: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#25

* Replace the usage of the _cps member with the bits copied over from nsContentPrefService.js.  That includes the HostnameGrouper prototype from nsContentPrefService.js and the "XPCOM Plumbing" at the bottom of that file.

2. Update nsContentPrefService.manifest

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/nsContentPrefService.manifest

Replace the "nsContentPrefService.js" references in this file with "ContentPrefService2.js".

3. Remove the old service

* Remove the idl file: https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPrefService.idl

* Remove the reference to that file here, in this moz.build file: https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/moz.build

* Remove this whole directory, which contains tests for the old service: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit

* Remove the reference to that directory here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/moz.build#8

* Remove ContentPrefInstance.jsm, since it's a companion to the old service, and there aren't any non-test consumers of it in m-c anymore: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefInstance.jsm

* Remove the reference to that file here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/moz.build#22

* Remove this test file: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit_cps2/test_service.js

* Remove the reference to that file here, in the tests manifest: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit_cps2/xpcshell.ini#7

4. Update places that reference the old service

* Find cpp files that #include nsIContentPrefService.h, remove the #include, and if the file does not already #include nsIContentPrefService2.h (note the 2), add an #include for it.  You can find those cpp files here (there are only two): https://dxr.mozilla.org/mozilla-central/search?q=%23include%20%22nsIContentPrefService.h%22

* This line should reference nsIContentPrefService2 (the new service), not nsIContentPrefService (the old one): https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Services.jsm#71

* Same here: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_Services.js#36

* Remove this other deprecated method that relies on the old service: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadLastDir.jsm#113

Comment 12

[Raj Meghpara]

okay sir,@adw Thanks a lot..!!

Comment 13

[Raj Meghpara]

hello sir @adw in your instructions :
          
Replace the usage of the _cps member with the bits copied over from nsContentPrefService.js.  That includes the HostnameGrouper prototype from nsContentPrefService.js and the "XPCOM Plumbing" at the bottom of that file.

In this instruction, what does _cps member mean ?Sir,Can you please explain this in detail ?

Comment 14

[Raj Meghpara]

Attachment: All changes are done..!!!

Hello sir,I've created this uncommited.patch file,please check it and suggest me anything is wrong..Thank you..!!

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8825048 [details] [diff] [review]
All changes are done..!!!

Requesting review on behalf of Raj.

Comment 16

[(no longer active)]

(qf:p3 since this is removing dead code as far as I can tell. ;-)

Comment 17

[Florian Quèze [:florian]]

Drew, do you still intend to mentor this? If this is indeed the way to resolve the startup perf hit I'm seeing in bug 1361095 comment 5, it's pretty important to resolve this for photon.

Comment 18

[Drew Willcoxon :adw]

Sure.

Comment 19

[Drew Willcoxon :adw]

Raj, sorry for not reviewing this.  Would you still like to work on it?

Comment 20

[Masatoshi Kimura [:emk]]

Raj, see comment #19. I'll take over if you don't respond within two weeks.

Comment 21

[Florian Quèze [:florian]]

(In reply to Masatoshi Kimura [:emk] from comment #20)
> Raj, see comment #19. I'll take over if you don't respond within two weeks.

Raj's last bugzilla activity was in January, I don't think you need to wait more to take it over.

To remind how important this bug is, here is a startup profile where this blocks us for 1617ms: https://perfht.ml/2uCYbfB

Comment 22

[Masatoshi Kimura [:emk]]

OK, taking.

Comment 23

[Masatoshi Kimura [:emk]]

Attachment: Bug 886907 - Remove old synchronous contentPrefService from the tree.

Review commit: https://reviewboard.mozilla.org/r/167592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167592/

Comment 24

[Masatoshi Kimura [:emk]]

Comment on attachment 8896331 [details]
Bug 886907 - Remove old synchronous contentPrefService from the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167592/diff/1-2/

Comment 25

[Masatoshi Kimura [:emk]]

Comment on attachment 8896331 [details]
Bug 886907 - Remove old synchronous contentPrefService from the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167592/diff/2-3/

Comment 26

[Masatoshi Kimura [:emk]]

Comment on attachment 8896331 [details]
Bug 886907 - Remove old synchronous contentPrefService from the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167592/diff/3-4/

Comment 27

[Drew Willcoxon :adw]

Comment on attachment 8896331 [details]
Bug 886907 - Remove old synchronous contentPrefService from the tree.

https://reviewboard.mozilla.org/r/167592/#review173724

Thanks Masatoshi, looks good.

Comment 28

[Drew Willcoxon :adw]

BTW there seems to be the idea in this bug that removing the old service alone will improve performance, but that's unlikely IMO, at least not significantly.  The perf problem is the main-thread storage/SQLite calls that the service makes on init, and this patch doesn't change that (rightly, since that should be done separately).

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f92c249cae8b
Remove old synchronous contentPrefService from the tree. r=adw

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing editor/libeditor/tests/test_bug1368544.html on Android 4.3 debug:

https://hg.mozilla.org/integration/autoland/rev/122fa30ec6605756d1221e91facd661e4b8069d7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f92c249cae8b855ff46035135785b73474bbc4ac&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123248311&repo=autoland

[task 2017-08-15T12:30:21.407119Z] 12:30:21     INFO -  78 INFO None79 INFO TEST-START | editor/libeditor/tests/test_bug1368544.html
[task 2017-08-15T12:35:32.798392Z] 12:35:32     INFO -  Buffered messages logged at 12:30:16
[task 2017-08-15T12:35:32.798477Z] 12:35:32     INFO -  80 INFO must wait for load
[task 2017-08-15T12:35:32.798718Z] 12:35:32     INFO -  Buffered messages finished
[task 2017-08-15T12:35:32.799494Z] 12:35:32     INFO -  81 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1368544.html | Test timed out.
[task 2017-08-15T12:35:32.800084Z] 12:35:32     INFO -      reportError@SimpleTest/TestRunner.js:121:7
[task 2017-08-15T12:35:32.800731Z] 12:35:32     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7

Comment 31

[Masatoshi Kimura [:emk]]

I have no idea why it fails...

Comment 32

[Masatoshi Kimura [:emk]]

Comment on attachment 8896331 [details]
Bug 886907 - Remove old synchronous contentPrefService from the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167592/diff/4-5/

Comment 33

[Masatoshi Kimura [:emk]]

Ah, I got it. Mobile has its own package-manifest.in.

Comment 34

[Masatoshi Kimura [:emk]]

Try looks good, relanding.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb321b223a034d795b0ccb61bb39ab638935d9b

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/832df1d6017e
Remove old synchronous contentPrefService from the tree. r=adw

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/832df1d6017e

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8e0c7bfb03ce
Port bug 886907 to TB/IB/SM: Package ContentPrefService2.js/manifest instead of nsContentPrefService.js/manifest. rs=bustage-fix