420677 - "Help > Report Web Forgery..." does nothing; "this.appContext is null"

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008030300 Minefield/3.0b4pre

Steps to reproduce:
1. Help > Report Web Forgery...

Result: "JavaScript error: chrome://browser/content/safebrowsing/sb-loader.js, line 32: this.appContext is null".  No visible UI changes.

Expected: Some way to report a web forgery.

(jX and philor have confirmed that it is broken, so it's not just me.)

Comment 1

[Reed Loden [:reed]]

Could always pull the useless-UI card for b4 if needed...

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

We'll relnote this for b4, not gonna hold the beta for it.

Comment 3

[Johnathan Nightingale [:johnath]]

This is a regression since beta 3; it is broken in the 0226 nightly, but works in 0225.  Based on that, it seems pretty likely that bug 400324 is the culprit, though it's not clear to me what exactly would be causing it.  I can confirm that safebrowsing.delayedStartup is being called, but by the time getReportURL is called, this.appContext is null.

Ryan? 

Comment 4

[Johnathan Nightingale [:johnath]]

Attachment: Stop caching appContext

This patch just reverts the change made in bug 400324 to cache appContext, instead re-getting it in getReportURL.  It fixes the problem, but I'm not sure what the motivation was for doing this in the first place, so I'm gonna tag Ryan for a review here, to see if I'm missing something.  There don't appear to be any external references to the field, nor any other uses in sb-loader.js.

Comment 5

[Johnathan Nightingale [:johnath]]

Attachment: simpler, better fix

The changed code was running deferredStartup directly via a setTimeout, meaning that it had the wrong 'this'.  One-liner to run deferredStartup with the right context.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

This means the menu item will be similarly broken in the first two seconds after startup, right? Not a major issue, to be sure, but seems like we should fix it... how about adding a getter that will retrieve appContext lazily, and using it from both deferredStartup and getReportURL? That way we avoid getting the service until deferredStartup in the common case, but things still work OK if the user's really eager :)

Comment 7

[Johnathan Nightingale [:johnath]]

Attachment: Add lazy-init getter

As requested, my liege.

If this terrible user exists and does this terrible thing, they will be fine now.  The code in getReportURL doesn't rely on the malware/phishing tables being initialize()'d either, so I'm leaving that to deferredStartup.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 307586 [details] [diff] [review]
Add lazy-init getter

Might be nice to use an actual getter, and the memoization technique from bug 385809 (e.g. http://mxr.mozilla.org/mozilla/source/toolkit/content/viewZoomOverlay.js#49 ).

Comment 9

[Johnathan Nightingale [:johnath]]

Attachment: Use getter

Good idea - I think your review still holds, but take a look?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 307598 [details] [diff] [review]
Use getter

Remove the space in "appContext ()"?

Comment 11

[Johnathan Nightingale [:johnath]]

Attachment: Spacing correction

I'ma go ahead and carry forward your review+

Comment 12

[Johnathan Nightingale [:johnath]]

Checking in browser/components/safebrowsing/content/sb-loader.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/sb-loader.js,v  <--  sb-loader.js
new revision: 1.28; previous revision: 1.27
done

Comment 13

[pal-moz]

wrong bug# in Description on bonsai.

Comment 14

[Johnathan Nightingale [:johnath]]

(In reply to comment #13)
> wrong bug# in Description on bonsai.

bug 421285.  Thanks for spotting this.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre.