Closed Bug 1571770 Opened 11 months ago Closed 11 months ago

URLDecorationAnnotationsService.jsm should not load Preferences.jsm

Categories

(Core :: Privacy: Anti-Tracking, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- verified
firefox70 --- verified

People

(Reporter: florian, Assigned: ehsan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

When running browser/base/content/test/performance/browser_startup.js locally, I get a failure saying:
FAIL resource://gre/modules/Preferences.jsm is not allowed before first paint

The cause is "resource://gre/modules/URLDecorationAnnotationsService.jsm":7:36

Looking at the code, the first problem that jumps at me is that https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/toolkit/components/antitracking/URLDecorationAnnotationsService.jsm#7 imports Preferences.jsm eagerly instead of lazily.

Then the second problem is that this new code is using the deprecated Preferences.jsm to do something that wouldn't be significantly harder to do without it: https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/toolkit/components/antitracking/URLDecorationAnnotationsService.jsm#33-38

And finally that leads to the question: why was the test failure not happening on infra? I just found the answer: antitracking.manifest was not added to https://searchfox.org/mozilla-central/source/browser/installer/package-manifest.in in bug 1568341, so this component only gets initialized during startup for non-packaged local builds.

I was going to say that this code really needs a test, but it actually does have a test, which forces the initialization of the service at https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/toolkit/components/antitracking/test/browser/browser_urlDecorationStripping.js#37-39, hiding the problem. These lines in the test file should be removed if this service is expected to be started automatically during startup.

Flags: needinfo?(ehsan)
Summary: URLDecorationAnnotationsService.jsm should load Preferences.jsm → URLDecorationAnnotationsService.jsm should not load Preferences.jsm

(In reply to Florian Quèze [:florian] from comment #0)

When running browser/base/content/test/performance/browser_startup.js locally, I get a failure saying:
FAIL resource://gre/modules/Preferences.jsm is not allowed before first paint

The cause is "resource://gre/modules/URLDecorationAnnotationsService.jsm":7:36

Sorry about that!

Looking at the code, the first problem that jumps at me is that https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/toolkit/components/antitracking/URLDecorationAnnotationsService.jsm#7 imports Preferences.jsm eagerly instead of lazily.

Then the second problem is that this new code is using the deprecated Preferences.jsm

Oh wow, Preferences.jsm is deprecated? I didn't know that. Should we make that obvious by, let's say, documenting that inside the file? :-)

to do something that wouldn't be significantly harder to do without it: https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/toolkit/components/antitracking/URLDecorationAnnotationsService.jsm#33-38

Oh OK. So the preference now is to use the raw XPCOM APIs in lieu of Preferences.jsm? I can certainly do that here...

And finally that leads to the question: why was the test failure not happening on infra? I just found the answer: antitracking.manifest was not added to https://searchfox.org/mozilla-central/source/browser/installer/package-manifest.in in bug 1568341, so this component only gets initialized during startup for non-packaged local builds.

Oops, that actually answers another question which I was trying to figure out the answer to right now... (I wish writing code that added files that needed to be packages didn't require a Master's Degree in Mozilla Packaging Sciences, I have repeatedly failed to obtain that degree throughout the years.)

I was going to say that this code really needs a test, but it actually does have a test, which forces the initialization of the service at https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/toolkit/components/antitracking/test/browser/browser_urlDecorationStripping.js#37-39, hiding the problem. These lines in the test file should be removed if this service is expected to be started automatically during startup.

The test for this service is fine as is. The reason why the test is written in the way that it is is to allow for the test to be run both as a stand-alone test (e.g. with the verify suite) and as part of the per-directory suites. When the test runs stand-alone, it may very well start well before the service has finished its initialization tasks, and the point behind await uds.ensureUpdated(); lines in the test is to eliminate that race condition...

Flags: needinfo?(ehsan)
Assignee: nobody → ehsan
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3947bf23458a
Part 1: Stop using Preferences.jsm inside URLDecorationAnnotationsService.jsm; r=florian,baku
https://hg.mozilla.org/integration/autoland/rev/7744a30c56a9
Part 2: Package antitracking.manifest in packaged builds; r=baku

(In reply to :Ehsan Akhgari from comment #1)

(In reply to Florian Quèze [:florian] from comment #0)

Oh wow, Preferences.jsm is deprecated? I didn't know that. Should we make that obvious by, let's say, documenting that inside the file? :-)

I would like to make this obvious by removing the file ;-). For now we only prevented loading it during startup before first paint. Last time I checked there were still a few consummers that remained in the tree and that were not entirely trivial to rewrite away from Preferences.jsm.

Oh OK. So the preference now is to use the raw XPCOM APIs in lieu of Preferences.jsm? I can certainly do that here...

Preferences.jsm was mostly a wrapper to work around pain points of the XPCOM API at a time when the XPCOM APIs were frozen. Now that they are no longer frozen we could fix the XPCOM API to reduce the pain (most notably support for unicode strings and support providing a default value instead of throwing when a pref doesn't exist).

(In reply to Florian Quèze [:florian] from comment #5)

(In reply to :Ehsan Akhgari from comment #1)

(In reply to Florian Quèze [:florian] from comment #0)

Oh wow, Preferences.jsm is deprecated? I didn't know that. Should we make that obvious by, let's say, documenting that inside the file? :-)

I would like to make this obvious by removing the file ;-). For now we only prevented loading it during startup before first paint. Last time I checked there were still a few consummers that remained in the tree and that were not entirely trivial to rewrite away from Preferences.jsm.

FWIW adding a giant // DO NOT USE THIS FILE IN NEW CODE ANY MORE comment to the beginning of this file would have caused me to not use it in the first place, as I was reading it to remind myself of its API when I was writing the original code that used it, since I was under the false impression that it is the nice way to access prefs from JS code. Up to you, but removing code that other developers keep adding new dependencies to is an uphill battle. :-)

(In reply to :Ehsan Akhgari from comment #6)

removing code that other developers keep adding new dependencies to is an uphill battle. :-)

I know. Almost won the battles for Task.jsm and Promise.jsm :-). For Preferences.jsm, it's harder to do an automated rewrite, so banning it during startup seems a good first measure.

Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Comment on attachment 9083403 [details]
Bug 1571770 - Part 2: Package antitracking.manifest in packaged builds;

This is required for the approval request for bug 1568341 (only part 2 of this bug) on beta. Please see that bug for more details.

In terms of risk, this patch only adds a file to the packaging manifest which was missing in bug 1568341 and on its own is virtually risk-free.

Attachment #9083403 - Flags: approval-mozilla-beta?

(In reply to :Ehsan Akhgari from comment #9)

This is required for the approval request for bug 1568341 (only part 2 of this bug) on beta. Please see that bug for more details.

You need both parts. If you uplift only part 2, browser_startup.js will fail.

In terms of risk, this patch only adds a file to the packaging manifest which was missing in bug 1568341 and on its own is virtually risk-free.

Adding this line to the package manifest makes a new module be loaded during startup before first paint. I don't see how this could be "virtually risk-free".

(In reply to Florian Quèze [:florian] from comment #10)

(In reply to :Ehsan Akhgari from comment #9)

This is required for the approval request for bug 1568341 (only part 2 of this bug) on beta. Please see that bug for more details.

You need both parts. If you uplift only part 2, browser_startup.js will fail.

Oh, very good point. I'll submit a request for the first part too.

In terms of risk, this patch only adds a file to the packaging manifest which was missing in bug 1568341 and on its own is virtually risk-free.

Adding this line to the package manifest makes a new module be loaded during startup before first paint. I don't see how this could be "virtually risk-free".

What you're saying is true (and that's a small part of the overall risk), but I've already talked about that in the approval request of bug 1568341. Apologies if this is confusing. This second patch should have really been included in the original series and had that happened all of this split conversation would have been avoided. :-/

Comment on attachment 9083402 [details]
Bug 1571770 - Part 1: Stop using Preferences.jsm inside URLDecorationAnnotationsService.jsm;

Beta/Release Uplift Approval Request

  • User impact if declined: This is required as part of the approval request for bug 1568341. If that approval request is to be granted, this should be granted as part of it for the patches to be able to land successfully. Otherwise it should probably be denied together with the rest.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See bug 1568341 comment 17.
  • List of other uplifts needed: Bug 1568341
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The risk of taking this patch on its own is small, because it is a refactoring to not use a specific JS module, and its correctness is verified using the associated automated test. But of course that's not important here since the overall risk profile of the whole patch series is what we should pay attention to and for that please see bug 1568341.
  • String changes made/needed: None
Attachment #9083402 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Regressions: 1429950
QA Whiteboard: [qa-triaged]

Reproduced the initial issue using an old Nightly build: 20190806112441
Verified - fixed on latest Nightly 70.0a1 (Build id: 20190808214929) on Windows 10, Ubuntu 18.04 and Mac OS 10.14

Comment on attachment 9083402 [details]
Bug 1571770 - Part 1: Stop using Preferences.jsm inside URLDecorationAnnotationsService.jsm;

Fixes a regression from bug 1568341. Approved for 69.0b13.

Attachment #9083402 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9083403 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Reproduced the initial issue using an old Beta build: 20190807220259
Verified - fixed on latest Beta 69.0b13 (64-bit) (Build id: 20190812173625) on Windows 10, Ubuntu 18.04 and Mac OS 10.14

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.