Closed Bug 420677 Opened 12 years ago Closed 12 years ago
"Help > Report Web Forgery
..." does nothing; "this .app Context is null"
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+
Target Milestone: Firefox 3 beta4 → Firefox 3
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?
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)
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.
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 :)
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 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+
Good idea - I think your review still holds, but take a look?
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+
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.
(In reply to comment #13) > wrong bug# in Description on bonsai. bug 421285. Thanks for spotting this.
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
You need to log in before you can comment on or make changes to this bug.