Safe Browsing notification banner does not disappear on next website

VERIFIED FIXED in Firefox 59

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: bodo, Assigned: steveisok)

Tracking

51 Branch
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 verified)

Details

Attachments

(2 attachments)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

I visited a website with unwanted download popup like: https://sourceforge.net/projects/imdisk-toolkit/?source=navbar

The popup appeared and I visited another normal website


Actual results:

the red popup appeared as planned but it didn't disappear after visiting a normal webpage


Expected results:

the popup should disappear in case a website with another 2nd level domain is visited
I can't reproduce it with FF51 on win7.
Component: Untriaged → Safe Browsing
Product: Firefox → Toolkit
The original warning is gone, here are more reliable steps to reproduce:

1. Load https://itisatrap.org/firefox/unwanted.html
2. Click "ignore this warning"
3. Click the "Firefox free download" green button

Expected:

The red notification bar at the top of the page should go away after loading the Firefox download page.

Actual:

The red notification bar sticks around even after leaving the itisatrap.org page.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Priority: -- → P3
Duplicate of this bug: 1361456
Summary: unwanted download popup does not disappear on next website → Safe Browsing notification banner does not disappear on next website
I've got a fix ready for an initial review.  Would it be easier/better to discuss with someone beforehand the approach and go from there?  Or should I just submit for review?
(In reply to Steve Pfister from comment #4)
> I've got a fix ready for an initial review.  Would it be easier/better to
> discuss with someone beforehand the approach and go from there?  Or should I
> just submit for review?

submit for review and the discussion can happen whilst the review is being done.
OK - submitted a patch for review.  I welcome any feedback and suggestions you have.  

I originally wanted to opt into the event when the location changes (and opt out), but ultimately adding a call to the one that already exists was a bit clearer.  

Also - the nsIEffectiveTLDService seemed like the right fit for determining when the 2nd level domain changes.  If there's something else or some other way, please suggest.
Attachment #8941814 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8941814 [details]
Bug 1338497 - Moved the Safe Browsing notification banner into its own object, made it aware of the current base domain, and added a function that's called when the location changes to determine if it should dismiss or not.

https://reviewboard.mozilla.org/r/212062/#review217850

Thanks for the patch! This is almost there, really. When you submit the next version, if you add ", r?gijs" at the end of the commit message, it'll automatically ask me for review when you push to mozreview.

::: browser/base/content/browser.js:8904
(Diff revision 1)
> +    var eTLDService = Components.classes["@mozilla.org/network/effective-tld-service;1"]
> +                          .createInstance(Components.interfaces.nsIEffectiveTLDService);

You almost never want to `createInstance` something called a 'service' - call `getService()` instead.

In this case, there's a handy shortcut you should use - just omit this declaration...

::: browser/base/content/browser.js:8908
(Diff revision 1)
> +
> +    var eTLDService = Components.classes["@mozilla.org/network/effective-tld-service;1"]
> +                          .createInstance(Components.interfaces.nsIEffectiveTLDService);
> +
> +    //start tracking host so that we know when we leave the domain
> +    this._currentURIBaseDomain = eTLDService.getBaseDomain(uri);

... and use `Services.eTLD.getBaseDomain(uri)` here.

::: browser/base/content/browser.js:8930
(Diff revision 1)
> +    // Persist the notification until the user removes so it
> +    // doesn't get removed on redirects.
> +    notification.persistence = -1;
> +  },
> +  onLocationChange(aLocationURI) {
> +    //take this to represent that you haven't visited a bad place

Nit: always spaces after '//'. Make sure you run eslint (`./mach eslint browser/base/content/browser.js`) on code. There's a commit hook you can set up as well, see https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook .

::: browser/base/content/browser.js:8935
(Diff revision 1)
> +    let eTLDService = Components.classes["@mozilla.org/network/effective-tld-service;1"]
> +                          .createInstance(Components.interfaces.nsIEffectiveTLDService);
> +    let newURIBaseDomain = eTLDService.getBaseDomain(aLocationURI);

Same issue with how you get to the eTLD service here.
Attachment #8941814 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8942062 [details]
Bug 1338497 - Incorporated review feedback.  Used existing service as opposed to creating it and put a space in between comments,

https://reviewboard.mozilla.org/r/212272/#review218132

Great, thanks, r=me!

Confusingly, unlike github, the mozreview workflow generally works by squashing commits when pushing after review feedback, rather than having "fix the review comments" commits stacked on top of the original ones while they're being reviewed and then squashing when actually landing the change. Reviewboard/mozreview takes care of keeping the old revs and giving you interdiffs and so on. It's nice!

In hg, you can fold commits with `hg fold` or `hg squash` or, for working directory changes, just plain `hg amend` / `hg commit --amend`. `hg histedit` is (I am told) equivalent to git's interactive rebase. For all of these, you probably need the `histedit` extension enabled in your `.hgrc` (or, on Windows, `mercurial.ini`) configuration files.

Because our idiosyncratic workflows shouldn't be your problem, I'll just combine these changes myself and land on inbound. Thanks for your help!
Attachment #8942062 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22130a72837b
make the safebrowsing notification dismiss when navigating to other domains, r=gijs
Assignee: nobody → steveisok
Status: NEW → ASSIGNED
Thanks for your help.  I'll get used to how this all works at some point.  

Since the change is in inbound, what additional steps do I need to take?  From what I read, it'll land in mozilla-central automatically.  Is that correct?
(In reply to Steve Pfister from comment #12)
> Thanks for your help.  I'll get used to how this all works at some point.  
> 
> Since the change is in inbound, what additional steps do I need to take? 
> From what I read, it'll land in mozilla-central automatically.  Is that
> correct?

Yep! Unless it gets backed out for some reason, you don't need to do anything else. This bug will get updated automatically (well, without action on our parts) when it lands on mozilla-central.
https://hg.mozilla.org/mozilla-central/rev/22130a72837b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
QA Whiteboard: [good first verify]
I have reproduced this bug on nightly according to (2017-02-10)

Fixing bug is verified on Latest Beta--
Build ID 	20180215111455
User Agent 	Mozilla/5.0 (Windows NT 6.1; rv:59.0) Gecko/20100101 Firefox/59.0 

After fixing, this issue is Solved.

Tested OS-- Windows7 32bit
QA Whiteboard: [good first verify] → [testday-20180216]
Reproduced this bug using the STR from comment 2, on an affected Nightly build (2017-02-10).

This is not repro anymore with latest Beta 59.0b11 on the following OSes: Win 7 x64, macOS 10.12 and Ubuntu 16.04 x64.

Thank you Saheda for your help!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.