Use content pref service to persist warned hosts

RESOLVED FIXED in Firefox 68

Status

()

enhancement
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

(Depends on 1 bug)

unspecified
Firefox 68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

4 months ago

Currently, we remember breached sites that we've warned the user about by putting them in a Set called warnedHosts[1], which is JSON'd into a pref[2].

This poses privacy concerns because the pref is not cleared when clearing history/private data, and especially because we are persisting hosts visited in private browsing mode.

While showing the prompt in private browsing mode is intentional, persisting those hosts in a pref is problematic.

Using the content pref service is a solution to both of these issues.

One thing that ISN'T clear, however, is how to address the situation for Release users who have already been running Firefox Monitor as a system add-on. At the moment I'd propose that the patch for this bug should check if the pref already has a value and if so, port the domains in that value into content prefs and then clear it.

Shipping migration code never feels clean though. We might consider using Normandy to deploy a new version of the add-on that does the clean up, which might allow us to avoid shipping migration code in mozilla-central.

Johann, do you have any further thoughts on this, and especially on migrating existing users?

[1] https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/browser/extensions/fxmonitor/privileged/FirefoxMonitor.jsm#13
[2] https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/browser/extensions/fxmonitor/privileged/FirefoxMonitor.jsm#16

Flags: needinfo?(jhofmann)

I'd say a simple piece of migratory code somewhere in startup-proximity that does the following:

if (Services.prefs.prefHasUserValue("extensions.fxmonitor.warnedHosts")) {
  migratePrefToContentPrefs();
  Services.prefs.clearUserPref("extensions.fxmonitor.warnedHosts");
}

should do the trick :)

That really doesn't look too hacky to me. We could probably remove the migratory code at some point, though there's no real harm in leaving it in as long as we think is appropriate.

Does that sound good to you?

Flags: needinfo?(jhofmann)
Assignee

Comment 2

4 months ago

(In reply to Johann Hofmann [:johannh] from comment #1)

I'd say a simple piece of migratory code somewhere in startup-proximity that does the following:

if (Services.prefs.prefHasUserValue("extensions.fxmonitor.warnedHosts")) {
  migratePrefToContentPrefs();
  Services.prefs.clearUserPref("extensions.fxmonitor.warnedHosts");
}

should do the trick :)

That really doesn't look too hacky to me. We could probably remove the migratory code at some point, though there's no real harm in leaving it in as long as we think is appropriate.

Does that sound good to you?

Yes, that's in alignment with what I was thinking. Thanks!

Assignee

Comment 6

4 months ago

Depends on D21554

Assignee

Comment 7

4 months ago

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

I predict a failure due to the test taking too long; we will probably have to requestLongerTimeout(). This test uses waitForCondition() multiple times to ensure a condition doesn't occur, and each occurrence takes a few seconds.

Assignee

Updated

4 months ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Assignee

Comment 8

4 months ago

(In reply to Nihanth Subramanya [:nhnt11] from comment #7)

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

I predict a failure due to the test taking too long; we will probably have to requestLongerTimeout(). This test uses waitForCondition() multiple times to ensure a condition doesn't occur, and each occurrence takes a few seconds.

So there is no failure due to it taking too long (though I wouldn't be surprised if an intermittent crops up at some point) but linux64 had a failure during the test - specifically after the first alert is shown, we reload and make sure an alert is not shown again for the same site - yet in this test, the alert was present.

I'm pretty sure this failure is due to us checking for the alert before the first one goes away when we reload - i.e. we call reload() and then start checking for the absence of the notification before the reload has actually caused the first notification to go away. I'll amend the patch and do another try push, this time with the whole test suite xpcshell tests as well because Johann pointed out more failures with a similar remote-settings-fake-data test for bug 1511111.

Edit:
Here's the new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab3f3f22590d6283322b237d9a1eb67c9ff6211

Assignee

Comment 9

3 months ago

I'm going to land this in 68 because the feature has already been QA'd and I don't want to lose that green flag.
The privacy concern is IMO not big enough to warrant trying to land this in 67.

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nhnt11, could you have a look please?

Flags: needinfo?(nhnt11)
Assignee

Comment 11

2 months ago

OK, the telemetry and pref-on bugs have landed. Here's a new try push for this bug before I try to land it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca680eee01d3089f7b190365bb52263c40cff0c4

Flags: needinfo?(nhnt11)
Attachment #9047414 - Attachment is obsolete: true
Assignee

Comment 13

2 months ago

The tests seem to be leaking windows and I can't figure out why. In the interest of landing some test coverage quickly, I vote we disable the test on debug for now and file a follow-up bug to re-enable it. I'm going to go ahead and make a patch and bug to this effect.

Assignee

Updated

2 months ago
Depends on: 1547517
Assignee

Comment 15

2 months ago

For posterity, here's a summary of my investigation into the leaking windows:

  • The failure is reproducible locally.
  • The log suggests a URL "_generated_background_page.html" - this is the "page" in whose scope the addon's background.js script runs.
  • FWIW, I couldn't find any instances of references to windows/documents beyond the appropriate lifetime.
  • Putting the contents of background.js in a closure didn't help.
  • Since the add-on is enabled, there must be something going on that's specifically relevant to this test - i.e. it's because of the doorhanger, or the test is keeping a reference it shouldn't, etc. Need to investigate further.
Assignee

Comment 16

2 months ago

New findings:

The warned hosts pref migration test reloads the addon. This is the culprit. The following code is sufficient to cause this leak:

add_task(async function foobar() {
  let addon = await AddonManager.getAddonByID("fxmonitor@mozilla.org");
  await addon.reload();
});

In light of this, I think it's ok to skip this test on debug for now, and investigate this issue further. I'm curious if this leak is reproducible with a minimal add-on (manifest + empty background script).

Comment 17

2 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d42051b63b49
Use content pref service to persist warned hosts. r=johannh
https://hg.mozilla.org/integration/autoland/rev/cb0bbf72229c
Migrate existing warnedHosts value to content prefs. r=johannh
https://hg.mozilla.org/integration/autoland/rev/09b33a0a01dc
Improve fxmonitor alert test coverage. r=johannh
https://hg.mozilla.org/integration/autoland/rev/6e4905f79080
Disable fxmonitor tests on debug. r=johannh
You need to log in before you can comment on or make changes to this bug.