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)
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)
883.27 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
johannh
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details |
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•7 years ago
|
Flags: needinfo?(francois)
Comment 1•7 years ago
|
||
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•7 years ago
|
Mentor: jhofmann
Reporter | ||
Comment 2•7 years 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.
Reporter | ||
Comment 4•7 years ago
|
||
Have fun! Let me know if you need anything!
Assignee: nobody → daisyts
Status: NEW → ASSIGNED
Reporter | ||
Comment 5•7 years ago
|
||
You can test the problem on https://itisatrap.org/firefox/its-a-trap.html
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Comment 6•7 years 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•7 years 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•7 years ago
|
Assignee: nobody → bianca.scura
Status: NEW → ASSIGNED
Reporter | ||
Comment 8•7 years ago
|
||
Done, let me know if you have any questions!
Assignee | ||
Comment 9•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Oh, sorry, I misread your reply. Sure I can push it for review!
Comment hidden (mozreview-request) |
Reporter | ||
Comment 14•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8851635 -
Attachment is obsolete: true
Attachment #8851635 -
Flags: review?(jhofmann)
Assignee | ||
Comment 23•7 years 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•7 years 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•7 years 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•7 years ago
|
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Flags: needinfo?(biancadanforth)
Keywords: checkin-needed
Comment 26•7 years ago
|
||
This was autolanded successfully, but pulsebot was down at the time and the bug never got updated.
Keywords: checkin-needed
Comment 27•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a2e19d3262c6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 29•7 years ago
|
||
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•7 years ago
|
||
(Let me know if you need help with that) :)
Assignee | ||
Comment 31•7 years 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•7 years 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 33•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/9503d9bfaf28
Updated•7 years ago
|
Flags: qe-verify+
Comment 35•7 years ago
|
||
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.
Description
•