Closed Bug 1277378 Opened 4 years ago Closed 4 years ago
Error: provider is undefined' because 'goog-unwanted-simple' list is missing after modifying safe browsing options and returning to default
58 bytes, text/x-review-board-request
Firefox 49.0a1 + 48.0a2 20160101 on Windows 8.1 After I set the safe browsing preferences in Firefox's Options to different states and then return to default one (all three boxes checked), the browser console reports: TypeError: provider is undefined this.SafeBrowsing.registerTableWithURLs() SafeBrowsing.jsm:85 this.SafeBrowsing.registerTables() SafeBrowsing.jsm:93 this.SafeBrowsing.readPrefs() SafeBrowsing.jsm:170 this.SafeBrowsing.init() SafeBrowsing.jsm:69 gBrowserInit._delayedStartup/<() browser.js:1144 SafeBrowsing.jsm:85:5 Setting a breakpoint in registerTableWithURLs reveals that 'goog-unwanted-simple' isn't a key of |this.listToProvider|, so providerName and provider are undefined and the error gets thrown for provider.updateURL when |listManager.registerTable(listname, providerName, provider.updateURL, provider.gethashURL);| gets called.
Oops, looks like I completely missed that typo when I reviewed bug 874408. The fix is to replace all instances of goog-unwanted-simple with goog-unwanted-shavar everywhere. [Tracking Requested - why for this release]: We need to track this for 48 and 49 because it's a serious regression. Safe Browsing updates will be broken as soon as a user changes the default preferences in the UI.
I encountered this today on one test profile, and had been trying to debug it. However, I can't seem to replicate the exact steps required to make it happen again. If anyone figures that part out, I'd be thankful, so that I can add tests for it.
(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #2) > I encountered this today on one test profile, and had been trying to debug > it. However, I can't seem to replicate the exact steps required to make it > happen again. In terms of reproducing the problem (i.e. the pref being set to the wrong thing), you can do this: 1. Open about:config in one window and search for "urlclassifier.malwareTable". Notice that it's the default value (non-bold). 2. In another window, open about:preferences and go to the security tab. 3. Untick "Warn me about unwanted and uncommon software" and notice that goog-unwanted-shavar and test-unwanted-simple have been removed. 4. Tick that same checkbox. Expected: The urlclassifier.malwareTable pref is back to its default value. Actual: What should be goog-unwanted-shavar in the pref value is actually misspelled as goog-unwanted-simple and so the pref is still in bold since it's not the default value anymore. Also, we never remove goog-unwanted-shavar when the option is unticked because we're looking for the invalid goog-unwanted-simple list.
Review commit: https://reviewboard.mozilla.org/r/57012/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57012/
Attachment #8758939 - Flags: review?(past)
Comment on attachment 8758939 [details] MozReview Request: Bug 1277378 - Fix typo in Safe Browsing in-content preferences. r?past https://reviewboard.mozilla.org/r/57012/#review53882 It's unfortunate that this is the kind of error that the test cannot catch.
Attachment #8758939 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #5) > It's unfortunate that this is the kind of error that the test cannot catch. Yeah, it's hard enough working with the 12+ hour delay while waiting for a full table update, let alone what happens when an action such as this (manipulating prefs) might affect the *next* table update. Very hard to catch in a traditional testing sense. However, the steps in comment 3 might make some sort of sanity regression test possible, at least for this bug. I will look into drafting this, and adding it to our existing safe browsing sanity tests that are run during build certification. We wouldn't catch a regression like this right away, but it would be prevented from going into beta at least... possibly sooner if the team that does test passes also certifies Aurora.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b1647e5d98a4 Fix typo in Safe Browsing in-content preferences. r=past
Comment on attachment 8758939 [details] MozReview Request: Bug 1277378 - Fix typo in Safe Browsing in-content preferences. r?past Approval Request Comment [Feature/regressing bug #]: Bug 874408 introduced this regression in Fx48. [User impact if declined]: Safe Browsing updates break if users touch the new "Warn me about unwanted and uncommon software" checkbox in about:preferences. No updates = no protection from Safe Browsing, so it's quite bad. [Describe test coverage new/current, TreeHerder]: There's a test which ensures that the checkbox logic works, but to ensure that the prefs it sets are the right ones, we rely on manual tests. [Risks and why]: Very low, it's just a typo. [String/UUID change made/needed]: None.
Attachment #8758939 - Flags: approval-mozilla-aurora?
Comment on attachment 8758939 [details] MozReview Request: Bug 1277378 - Fix typo in Safe Browsing in-content preferences. r?past Simple fix, and we want safe browsing to not be broken in beta 1. Matt would you mind verifying this in aurora once it lands?
Attachment #8758939 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on today's Fx48/49 using steps from comment 3.
You need to log in before you can comment on or make changes to this bug.