1338497 - Safe Browsing notification banner does not disappear on next website

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P3)

Description

[Bodo]

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

Comment 1

[Loic]

I can't reproduce it with FF51 on win7.

Comment 2

[François Marier [:francois]]

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.

Comment 4

[Steve Pfister]

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?

Comment 5

[Ludovic Hirlimann [:Usul]]

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

Comment 6

[Steve Pfister]

Attachment: 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.

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

Comment 7

[Steve Pfister]

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.

Comment 8

[:Gijs (he/him)]

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.

Comment 9

[Steve Pfister]

Attachment: Bug 1338497 - Incorporated review feedback. Used existing service as opposed to creating it and put a space in between comments,

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

Comment 10

[:Gijs (he/him)]

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!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22130a72837b
make the safebrowsing notification dismiss when navigating to other domains, r=gijs

Comment 12

[Steve Pfister]

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?

Comment 13

[:Gijs (he/him)]

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

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/22130a72837b

Comment 15

[Saheda Reza [:Antora]]

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

Comment 16

[Ciprian Georgiu, Desktop QA]

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!