Closed Bug 1528978 Opened 5 years ago Closed 5 years ago

Use content pref service to persist warned hosts

Categories

(Firefox :: Firefox Monitor, enhancement)

Product:
Firefox
Component:
Firefox Monitor
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1528978 - Use content pref service to persist warned hosts. r=johannh
47 bytes, text/x-phabricator-request
Details | Review
Bug 1528978 - Migrate existing warnedHosts value to content prefs. r=johannh
47 bytes, text/x-phabricator-request
Details | Review
Bug 1528978 - Improve fxmonitor alert test coverage. r=johannh
47 bytes, text/x-phabricator-request
Details | Review
Bug 1528978 - Disable fxmonitor tests on debug. r=johannh
47 bytes, text/x-phabricator-request
Details | Review

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)

(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!

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: nobody → nhnt11
Status: NEW → ASSIGNED

(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

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)

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

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.

Depends on: 1547517

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.

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).

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.

Attachment

General

Creator:
Created:
Updated:
Size: