Closed Bug 420677 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P1, major)

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: jruderman, Assigned: johnath)

References

Details

(Keywords: regression, relnote)

Attachments

(1 file, 4 obsolete files)

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.)
Flags: blocking-firefox3?
Could always pull the useless-UI card for b4 if needed...
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Firefox 3 beta4
We'll relnote this for b4, not gonna hold the beta for it.
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: relnote
Target Milestone: Firefox 3 beta4 → Firefox 3
Assignee: nobody → 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? 
Blocks: 400324
Attached patch Stop caching appContext (obsolete) — Splinter Review
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.
Attachment #307549 - Flags: review?
Attachment #307549 - Flags: review? → review?(rflint)
Attached patch simpler, better fix (obsolete) — Splinter Review
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.
Attachment #307549 - Attachment is obsolete: true
Attachment #307557 - Flags: review?(gavin.sharp)
Attachment #307549 - Flags: review?(rflint)
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 :)
Attached patch Add lazy-init getter (obsolete) — Splinter Review
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.
Attachment #307557 - Attachment is obsolete: true
Attachment #307586 - Flags: review?(gavin.sharp)
Attachment #307557 - Flags: review?(gavin.sharp)
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 ).
Attachment #307586 - Flags: review?(gavin.sharp) → review+
Attached patch Use getter (obsolete) — Splinter Review
Good idea - I think your review still holds, but take a look?
Attachment #307586 - Attachment is obsolete: true
Attachment #307598 - Flags: review?(gavin.sharp)
Comment on attachment 307598 [details] [diff] [review]
Use getter

Remove the space in "appContext ()"?
Attachment #307598 - Flags: review?(gavin.sharp) → review+
I'ma go ahead and carry forward your review+
Attachment #307598 - Attachment is obsolete: true
Attachment #307599 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
wrong bug# in Description on bonsai.
Depends on: 421285
(In reply to comment #13)
> wrong bug# in Description on bonsai.

bug 421285.  Thanks for spotting this.
Duplicate of this bug: 422306
Verified fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre.
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.