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

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Safe Browsing
P3
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: johannh, Assigned: bdanforth, Mentored)

Tracking

({good-first-bug, regression})

54 Branch
mozilla55
good-first-bug, regression
Points:
---

Firefox Tracking Flags

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

Details

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

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!
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
Mentor: jhofmann@mozilla.com
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

2 months ago
I would like to work on this.  Please assign it to me.
Have fun! Let me know if you need anything!
Assignee: nobody → daisyts
Status: NEW → ASSIGNED
You can test the problem on https://itisatrap.org/firefox/its-a-trap.html
Priority: -- → P3
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

a month ago
Hi Johann,

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

Thanks,
Bianca
Assignee: nobody → bianca.scura
Status: NEW → ASSIGNED
Done, let me know if you have any questions!
(Assignee)

Comment 9

a month 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
(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

a month 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

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

Comment 14

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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!
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

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

Comment 23

a month 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

a month 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

a month 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+
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

a month 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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2e19d3262c6
Status: ASSIGNED → RESOLVED
Last Resolved: a month 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
(Let me know if you need help with that) :)
(Assignee)

Comment 31

a month 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

a month 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

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/9503d9bfaf28
status-firefox54: affected → fixed
You need to log in before you can comment on or make changes to this bug.