Closed Bug 1347493 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P3)

54 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- verified
firefox55 --- verified

People

(Reporter: johannh, Assigned: bdanforth, Mentored)

References

Details

(Keywords: good-first-bug, regression)

Attachments

(2 files, 1 obsolete file)

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
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.
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
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!
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.
(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?
Oh, sorry, I misread your reply. Sure I can push it for 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-
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.
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.
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. :)
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 :)
(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)
Attachment #8851635 - Attachment is obsolete: true
Attachment #8851635 - Flags: review?(jhofmann)
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)
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)
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+
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
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
https://hg.mozilla.org/mozilla-central/rev/a2e19d3262c6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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!
Flags: needinfo?(biancadanforth)
Version: unspecified → 54 Branch
(Let me know if you need help with that) :)
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?
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+
Flags: qe-verify+
Verified as fixed in Nightly 55.0a1 (id: 20170504030320) and Beta 54.0b4.
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.