Last Comment Bug 813691 - Phishing toolbar needs to be updated to use new nsIUrlListManager interface
: Phishing toolbar needs to be updated to use new nsIUrlListManager interface
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 20.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks: 811193
  Show dependency treegraph
 
Reported: 2012-11-20 12:06 PST by Magnus Melin
Modified: 2012-12-23 13:40 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1 (2.38 KB, patch)
2012-11-21 19:24 PST, Mike Conley (:mconley) - (Needinfo me!)
standard8: review+
Details | Diff | Splinter Review
Patch v2 (r+'d by Standard8) (2.20 KB, patch)
2012-11-22 06:57 PST, Mike Conley (:mconley) - (Needinfo me!)
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Magnus Melin 2012-11-20 12:06:57 PST
Seen starting https://hg.mozilla.org/comm-central/rev/6996e05c74ee but that hardly caused it. Some core change perhaps?

SUMMARY-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\message-header\test-phishing-bar.js | test-phishing-bar.js::test_phishing_bar_for_eml_attachment
  EXCEPTION: Notification bar is collapsed!
    at: test-folder-display-helpers.js line 2831
       assert_true test-folder-display-helpers.js 2831
       assert_phishing_bar_visible test-phishing-bar.js 42
       test_phishing_bar_for_eml_attachment test-phishing-bar.js 89
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-11-21 07:46:08 PST
I'm bisecting this one today.
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-11-21 11:38:38 PST
Bisecting this has revealed it to be fallout from bug 811193.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-11-21 11:40:23 PST
So, if I'm reading this correctly - this didn't just break a test; this has actually busted our ability to detect phishing emails. Nice.

Looks like just an interface change, anyhow.
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-11-21 11:43:46 PST
I'm not entirely sure what needs to happen here to fix our phishing detection. Clearly, because of https://bugzilla.mozilla.org/attachment.cgi?id=681049&action=diff , we need to change what we're passing to safeLookup.

Any idea what we're supposed to do to change the key from an ACString to an nsIPrincipal?
Comment 5 Mark Banner (:standard8) 2012-11-21 15:24:42 PST
Bug 775796 may provide some hints
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-11-21 19:24:23 PST
Created attachment 684284 [details] [diff] [review]
Patch v1

This fixes the test for me, and causes the phishing toolbar to appear.

Try builds coming here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1ec926fe3faa
Comment 7 Magnus Melin 2012-11-21 22:37:31 PST
Comment on attachment 684284 [details] [diff] [review]
Patch v1

Review of attachment 684284 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/phishing/list-warden.js
@@ +164,5 @@
>    this.debugZone = "multitablequerier";
> +
> +  let uri = Services.io.newURI(url, null, null);
> +  this.principal_ = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
> +                              .getService(Components.interfaces

Services.scriptSecurityManager
Comment 8 Mark Banner (:standard8) 2012-11-22 04:32:47 PST
Comment on attachment 684284 [details] [diff] [review]
Patch v1

Review of attachment 684284 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-11-22 06:39:09 PST
(In reply to Magnus Melin from comment #7)
> Comment on attachment 684284 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 684284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/phishing/list-warden.js
> @@ +164,5 @@
> >    this.debugZone = "multitablequerier";
> > +
> > +  let uri = Services.io.newURI(url, null, null);
> > +  this.principal_ = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
> > +                              .getService(Components.interfaces
> 
> Services.scriptSecurityManager

Ah, way better. Wasn't aware that the script security manager was in there. Thanks Magnus!
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-11-22 06:57:07 PST
Created attachment 684413 [details] [diff] [review]
Patch v2 (r+'d by Standard8)

Fixed patch
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-11-22 06:59:53 PST
https://hg.mozilla.org/comm-central/rev/f402639db541
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2012-11-28 20:34:20 PST
Comment on attachment 684413 [details] [diff] [review]
Patch v2 (r+'d by Standard8)

Looks like bug 811193 got landed on mozilla-aurora, so that's busted us there.
Comment 13 Mark Banner (:standard8) 2012-12-23 13:40:38 PST
http://hg.mozilla.org/releases/comm-aurora/rev/c59a69b8db63

Note You need to log in before you can comment on or make changes to this bug.