1246115 - "This isn't a web forgery…" never appear for webpages detected as forgery

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[YUKI "Piro" Hiroshi]

The menu item "This isn't a web forgery…" never appear.

Steps to reproduce:

 1. Go to an webpage detected as forgery, like:
    http://itisatrap.org/firefox/its-a-trap.html
 2. Open the "Help" menu.

Actual result:

 There is "Report Web Forgery…" menuitem to report a new forgery.

Expected result:

 There is "This isn't a web forgery…" menuitem to report a wrong detection.


The command "This isn't a web forgery…" is aimed to be activated for an webpage detected as a forgery by the code:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-safebrowsing.js#13
However, because "gBrowser.currentURI" returns the actual URI of the page instead of the internal error page "about:blocked?e=phishingBlocked", the module always detect the current page as "not an internal error page".

Comment 1

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=96fd68f9c83e&tochange=5b89084d3dd6

Regressed by: Bug 1057966

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Apparently an e10s bug regressed this, but I'm guessing that this affects both e10s and non-e10s.

Adding to our triage queue in case someone on the team wants to step up.

Comment 3

[YUKI "Piro" Hiroshi]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2)
> Apparently an e10s bug regressed this, but I'm guessing that this affects
> both e10s and non-e10s.

Yes, I saw this problem on Firefox 44 without e10s.

I've verified that the document's "document.URL" reports correct internal URI starting with "about:blocked?e=phishingBlocked", via the Web Console. So I think we have to send the information from the frame script via the message manager.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2)
> Apparently an e10s bug regressed this, but I'm guessing that this affects
> both e10s and non-e10s.
> 
> Adding to our triage queue in case someone on the team wants to step up.

Regression, so it should block IMO.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?Gijs

Unfortunately, when onLocationChange is fired for an attack site for
the about:blocked error page that we display, content.document has not
been updated with the loaded error document, so
content.document.documentURI will appear to be the previous page that
had been loaded. In this patch, we update the parent's cache of
documentURI in onStateChange as well, since this seems to be fired
after the error page has been loaded.

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

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8718074 [details]
MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34381/diff/1-2/

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1246115 - Re-write and re-enable regression test. r?Gijs

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

Comment 8

[:Gijs (he/him)]

Comment on attachment 8718074 [details]
MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?Gijs

https://reviewboard.mozilla.org/r/34381/#review31289

::: browser/base/content/browser-safebrowsing.js:11
(Diff revision 2)
>      // A phishing page will have a specific about:blocked content documentURI

Can you make this comment still more explicit and say something like "We can't use `gBrowser.currentURI` here because in the case of a phishing page, it would point to the page the user tried to load, rather than the network error page (about:blocked)" ?

::: toolkit/content/browser-child.js:130
(Diff revision 2)
> +      json.documentURI = content.document.documentURIObject.spec;

Updating this earlier is going to be "interesting" in terms of consequences. Let's hope that our tests are good enough to catch issues with that...

Comment 9

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34381/#review31291

::: toolkit/content/browser-child.js:126
(Diff revision 2)
> +    // loading an internal error page, for which the parent
> +    // will want to know some details about, so we'll update

Late nit: "for which the parent will want to know some details about" needs to lose the "about".

Comment 10

[:Gijs (he/him)]

This test shouldn't have been disabled since bug 982988, and it seems that the following merge re-disabled it for some reason. It's not really clear why, and that happened in 2014 and nobody noticed. :-(

Comment 11

[:Gijs (he/him)]

See https://hg.mozilla.org/integration/fx-team/rev/cbce1167c712 and its kid, https://hg.mozilla.org/integration/fx-team/rev/9b1812a7852e

Comment 12

[:Gijs (he/him)]

Comment on attachment 8718448 [details]
MozReview Request: Bug 1246115 - Re-write and re-enable regression test. r?Gijs

https://reviewboard.mozilla.org/r/34581/#review31297

LGTM. Looks like it got re-disabled by accident in 2014, so let's assume the itsatrap stuff works correctly still and doesn't hit the network. (The try push looks good!)

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8718074 [details]
MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34381/diff/2-3/

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8718448 [details]
MozReview Request: Bug 1246115 - Re-write and re-enable regression test. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34581/diff/1-2/

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ced51b8fdda
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3b953ac30f

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b020a0014641

Comment 17

[Phil Ringnalda (:philor)]

Redisabled on e10s because https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8a3b953ac30f&group_state=expanded&filter-searchStr=e10s%20browser-chrome - given the relative percentages, I'd guess it races and loses more the slower the platform is.

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6ced51b8fdda
https://hg.mozilla.org/mozilla-central/rev/8a3b953ac30f
https://hg.mozilla.org/mozilla-central/rev/b020a0014641

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Hey philor,

I'm curious about the leave-open on this one. I understand (and am grateful) that you saw and disabled the test frequently failing in e10s mode. Are you okay with me closing this bug out and opening a new one to re-enable the test?

Comment 20

[Phil Ringnalda (:philor)]

Whatever works for you is fine by me: I think I had the impression this was an e10s bug, even though it makes it clear it isn't, but mostly I was just looking for the quickest way out so I didn't wind up working past midnight for the fourth day in a row.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Thanks philor - filed bug 1248632 to re-enable the test for e10s.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8718074 [details]
MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?Gijs

Approval Request Comment
[Feature/regressing bug #]:

E10s broke this for both e10s and non-e10s, way back in the day in bug 1057966.

[User impact if declined]:

Users that are visiting a reported attack site will not have easy access to a Help menu item that allows them to report that the site is _not_ an attack site.

[Describe test coverage new/current, TreeHerder]:

Re-activated some regression tests here, but they've been re-disabled for e10s. They're enabled for non-e10s, however. Filed bug 1248632 to get the tests turned on for e10s.

[Risks and why]: 

Low-risk. We were basically just checking the wrong things before, and now we're checking the right things. The added test coverage also boosts my confidence.

[String/UUID change made/needed]:

None.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8718448 [details]
MozReview Request: Bug 1246115 - Re-write and re-enable regression test. r?Gijs

Approval Request Comment:

See above (though this test will be disabled for e10s until bug 1248632 is fixed)

Comment 24

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Tracking since this is a fairly recent regression

Comment 25

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8718074 [details]
MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?Gijs

Fix for safebrowsing regression, with new tests. 
Please uplift to aurora

Comment 26

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8718448 [details]
MozReview Request: Bug 1246115 - Re-write and re-enable regression test. r?Gijs

New tests for recent e10s related regression in safebrowsing. 
OK to uplift to aurora.

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Comment on attachment 8718448 [details]
> MozReview Request: Bug 1246115 - Re-write and re-enable regression test.
> r?Gijs
> 
> New tests for recent e10s related regression in safebrowsing. 
> OK to uplift to aurora.

this has problems uplifting to aurora, could you take a look ?

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Yeah - I was conflicting on bug 1245649 which changed some whitespace and didn't land on aurora. Resolved and pushed:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d02f6fb2a68c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9b4821cbe633
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/035cb46081f4

Comment 29

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Yes

Actual Results: 
As expected

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1246115.html