undefined is the title of the Safe Browsing ignored warning notification-box

VERIFIED FIXED in Firefox 54

Status

()

Toolkit
Safe Browsing
P3
normal
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: johannh, Assigned: bdanforth, Mentored)

Tracking

({good-first-bug, regression})

54 Branch
mozilla55
good-first-bug, regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox53 unaffected, firefox54 verified, firefox55 verified, firefox-esr52 unaffected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 months ago
Created attachment 8847554 [details]
Screen Shot 2017-03-15 at 12.05.53.png

See screenshot. Only for phishing and malware, not for unwanted.

Francois, is this already tracked somewhere? If not I would turn this bug into a good-first-bug since this is trivial to fix.

Thanks!
(Reporter)

Updated

3 months ago
Flags: needinfo?(francois)
I don't think we're tracking this anywhere, but you're right, it's a regression from linked bug.
Flags: needinfo?(francois)
Keywords: good-first-bug
(Reporter)

Updated

3 months ago
Mentor: jhofmann@mozilla.com
(Reporter)

Comment 2

3 months ago
The problem here is that the title variable[0] is not assigned in all clauses of the if statement below. It should get assigned to a translated string in the same way it's done in the else branch[1].

You can find the translation strings here: http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/browser/locales/en-US/chrome/browser/browser.properties#512

[0] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/browser/base/content/browser.js#3165
[1] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/browser/base/content/browser.js#3195

Let me know if you have any questions.

Comment 3

3 months ago
I would like to work on this.  Please assign it to me.
(Reporter)

Comment 4

3 months ago
Have fun! Let me know if you need anything!
Assignee: nobody → daisyts
Status: NEW → ASSIGNED
(Reporter)

Comment 5

3 months ago
You can test the problem on https://itisatrap.org/firefox/its-a-trap.html
Priority: -- → P3
(Reporter)

Comment 6

3 months ago
Hi Daisy,

I'm unassigning you for now since this should ideally be solved in the next weeks. Feel free to comment if you'd like to pick this up again or have already made some progress.

Thanks!

Johann
Assignee: daisyts → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 7

3 months ago
Hi Johann,

I would like to fix this bug please! I finally got the Firefox build working on my local machine.

Thanks,
Bianca
(Reporter)

Updated

3 months ago
Assignee: nobody → bianca.scura
Status: NEW → ASSIGNED
(Reporter)

Comment 8

3 months ago
Done, let me know if you have any questions!
(Assignee)

Comment 9

3 months ago
Hi Johann,

I made the change and would like to test it. I read about Try Servers, but I need a certain level of commit access. As a new user, where could I find information on the workflow for testing my changes?

Thanks,
Bianca
(Reporter)

Comment 10

3 months ago
(In reply to Bianca Danforth [:bdanforth] from comment #9)
> Hi Johann,
> 
> I made the change and would like to test it. I read about Try Servers, but I
> need a certain level of commit access. As a new user, where could I find
> information on the workflow for testing my changes?
> 
> Thanks,
> Bianca

Yes, you have to apply for try push access (https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server). This is usually done after having fixed one or two bugs. In the meantime, if you upload your patch, I can do the try push for you.

I think for this simple change we're fine without adding a new test (but it's probably still a good idea to run the existing tests on try).

Let me know if I can help you with getting the patch uploaded.
(Assignee)

Comment 11

3 months ago
(In reply to Johann Hofmann [:johannh] from comment #10)
> (In reply to Bianca Danforth [:bdanforth] from comment #9)
> > Hi Johann,
> > 
> > I made the change and would like to test it. I read about Try Servers, but I
> > need a certain level of commit access. As a new user, where could I find
> > information on the workflow for testing my changes?
> > 
> > Thanks,
> > Bianca
> 
> Yes, you have to apply for try push access
> (https://wiki.mozilla.org/ReleaseEngineering/
> TryServer#Getting_access_to_the_Try_Server). This is usually done after
> having fixed one or two bugs. In the meantime, if you upload your patch, I
> can do the try push for you.
> 
> I think for this simple change we're fine without adding a new test (but
> it's probably still a good idea to run the existing tests on try).
> 
> Let me know if I can help you with getting the patch uploaded.

Okay, I've submitted a request for level 1 access. Could you vouch for me here: https://bugzilla.mozilla.org/show_bug.cgi?id=1350462?
(Assignee)

Comment 12

3 months ago
Oh, sorry, I misread your reply. Sure I can push it for review!
Comment hidden (mozreview-request)
(Reporter)

Comment 14

3 months ago
mozreview-review
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126118

Looks good, one minor thing:

::: browser/base/content/browser.js:3185
(Diff revision 1)
>      }];
>  
>      let title;
>      if (reason === "malware") {
>        let reportUrl = gSafeBrowsing.getReportURL("MalwareMistake", blockedInfo);
> -
> +      title = gNavigatorBundle.getString("safebrowsing.deceptiveSite");

I think this should be safebrowsing.reportedAttackSite instead :)
Attachment #8851154 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 15

3 months ago
mozreview-review-reply
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126118

> I think this should be safebrowsing.reportedAttackSite instead :)

Thank you for reviewing! I updated the value of title for malware sites in my latest commit.
(Reporter)

Comment 16

3 months ago
mozreview-review-reply
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126118

> Thank you for reviewing! I updated the value of title for malware sites in my latest commit.

Hm, did you do "hg push review" again? I don't see the diff on reviewboard.
(Assignee)

Comment 17

3 months ago
mozreview-review-reply
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126118

> Hm, did you do "hg push review" again? I don't see the diff on reviewboard.

Of course! Just now. :)
(Reporter)

Comment 18

3 months ago
mozreview-review-reply
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126118

> Of course! Just now. :)

Then you probably have to hit "publish" in the reviewboard UI or in the command line after pushing :)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

3 months ago
(In reply to Johann Hofmann [:johannh] from comment #18)
> Comment on attachment 8851154 [details]
> Bug 1347493 - Correctly assign a value to 'title' for phishing and malware
> sites
> 
> https://reviewboard.mozilla.org/r/123534/#review126118
> 
> > Of course! Just now. :)
> 
> Then you probably have to hit "publish" in the reviewboard UI or in the
> command line after pushing :)

Done!
(Reporter)

Comment 21

3 months ago
Hm, this looks good but it doesn't collapse your two commits into one reviewable commit on reviewboard. Would you mind running a histedit (like git rebase, see https://www.mercurial-scm.org/wiki/HisteditExtension) and joining your two commits?

Let me know if you have any questions around that! (Maybe better on IRC)
Flags: needinfo?(biancadanforth)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8851635 - Attachment is obsolete: true
Attachment #8851635 - Flags: review?(jhofmann)
(Assignee)

Comment 23

3 months ago
mozreview-review
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126984
Attachment #8851154 - Flags: review?(biancadanforth)
(Assignee)

Comment 24

3 months ago
mozreview-review
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126986
Attachment #8851154 - Flags: review?(biancadanforth)
(Reporter)

Comment 25

3 months ago
mozreview-review
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

https://reviewboard.mozilla.org/r/123534/#review126268

Looks great now, thanks!

Now this needs a run on our try server, to check if tests are still green. (https://wiki.mozilla.org/ReleaseEngineering/TryServer). I will do that for you, but in the future you might want to get your own try access. I'll also set the checkin-needed flag. This flag signals that we want the patch merged. If the try looks good, someone will come around and check it in for us.
Attachment #8851154 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

3 months ago
status-firefox54: --- → affected
status-firefox55: --- → affected
Flags: needinfo?(biancadanforth)
Keywords: checkin-needed
This was autolanded successfully, but pulsebot was down at the time and the bug never got updated.
Keywords: checkin-needed

Comment 27

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2e19d3262c6
Correctly assign a value to 'title' for phishing and malware sites r=johannh

Comment 28

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2e19d3262c6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thanks for fixing this, Bianca! One final request, can you please request Aurora approval on this patch so we can get it fixed on Firefox 54 as well? Simply click the "Details" link for your patch, change the approval‑mozilla‑aurora dropdown to '?', and answer the questions that get inserted into the comment field. Thanks!
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(biancadanforth)
Version: unspecified → 54 Branch
(Reporter)

Comment 30

3 months ago
(Let me know if you need help with that) :)
(Assignee)

Comment 31

3 months ago
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

Approval Request Comment
[Feature/Bug causing the regression]: 1347493
[User impact if declined]: User will see 'undefined' on the title bar when a dangerous site warning is ignored
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: Go to <url>, ignore the warning and look at the title bar at the top, will say 'undefined'
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple frontend change that added a missing variable assignment, runs very isolated.
[String changes made/needed]: None
Flags: needinfo?(biancadanforth)
Attachment #8851154 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 32

3 months ago
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

Whoops, I meant to say https://itisatrap.org/firefox/its-a-trap.html in place of <url>.
Comment on attachment 8851154 [details]
Bug 1347493 - Correctly assign a value to 'title' for phishing and malware sites

fix ui regression in aurora54
Attachment #8851154 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 34

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/9503d9bfaf28
status-firefox54: affected → fixed
Flags: qe-verify+
Verified as fixed in Nightly 55.0a1 (id: 20170504030320) and Beta 54.0b4.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
status-firefox55: fixed → verified
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.