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

VERIFIED FIXED in Firefox 3

Status

()

Toolkit
Safe Browsing
P1
major
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: johnath)

Tracking

({regression, relnote})

Trunk
Firefox 3
regression, relnote
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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

Updated

9 years ago
Assignee: nobody → johnath
(Assignee)

Comment 3

9 years ago
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? 
(Reporter)

Updated

9 years ago
Blocks: 400324
(Assignee)

Comment 4

9 years ago
Created attachment 307549 [details] [diff] [review]
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.
Attachment #307549 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #307549 - Flags: review? → review?(rflint)
(Assignee)

Comment 5

9 years ago
Created attachment 307557 [details] [diff] [review]
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.
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 :)
(Assignee)

Comment 7

9 years ago
Created attachment 307586 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

9 years ago
Created attachment 307598 [details] [diff] [review]
Use getter

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+
(Assignee)

Comment 11

9 years ago
Created attachment 307599 [details] [diff] [review]
Spacing correction

I'ma go ahead and carry forward your review+
Attachment #307598 - Attachment is obsolete: true
Attachment #307599 - Flags: review+
Attachment #307599 - Flags: review+
Attachment #307599 - Flags: review+
(Assignee)

Comment 12

9 years ago
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
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 13

9 years ago
wrong bug# in Description on bonsai.
(Assignee)

Updated

9 years ago
Depends on: 421285
(Assignee)

Comment 14

9 years ago
(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
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.