Closed Bug 1246115 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s m9+ ---
firefox44 --- affected
firefox45 --- affected
firefox46 + fixed
firefox47 + fixed
firefox-esr38 --- affected
firefox-esr45 --- ?

People

(Reporter: yuki, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(3 files)

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".
Flags: needinfo?(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.
tracking-e10s: --- → ?
Flags: needinfo?(mconley)
(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.
(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.
Assignee: nobody → mconley
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 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/
Attachment #8718074 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)
Attachment #8718448 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)
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...
Attachment #8718074 - Flags: review?(gijskruitbosch+bugs) → review+
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".
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 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!)
Attachment #8718448 - Flags: review?(gijskruitbosch+bugs) → review+
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/
Attachment #8718074 - Attachment description: MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?felipe → MozReview Request: Bug 1246115 - Make gSafeBrowsing set the phishing menu item correctly. r?Gijs
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/
Attachment #8718448 - Attachment description: MozReview Request: Bug 1246115 - Re-write and re-enable regression test. r?felipe → MozReview Request: Bug 1246115 - Re-write and re-enable regression test. r?Gijs
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.
Keywords: leave-open
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?
Flags: needinfo?(philringnalda)
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.
Flags: needinfo?(philringnalda)
Thanks philor - filed bug 1248632 to re-enable the test for e10s.
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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.
Attachment #8718074 - Flags: approval-mozilla-aurora?
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)
Attachment #8718448 - Flags: approval-mozilla-aurora?
Tracking since this is a fairly recent regression
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
Attachment #8718074 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8718448 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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 ?
Flags: needinfo?(mconley)
[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
You need to log in before you can comment on or make changes to this bug.