Closed Bug 1331873 Opened 3 years ago Closed 3 years ago

First run page for data choices causes JS error

Categories

(Firefox :: General, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: Gijs, Assigned: dao)

References

Details

Attachments

(1 file)

From bug 1331800:

can't access lexical declaration `tab' before initialization  TelemetryReportingPolicy.jsm:440
	_openFirstRunPage/progressListener.onStateChange resource://gre/modules/TelemetryReportingPolicy.jsm:440:1
	callListeners chrome://browser/content/tabbrowser.xml:490:24
	_callProgressListeners chrome://browser/content/tabbrowser.xml:511:13
	mTabProgressListener/<._callProgressListeners chrome://browser/content/tabbrowser.xml:560:22
	mTabProgressListener/<.onStateChange chrome://browser/content/tabbrowser.xml:729:15
	_loadURIWithFlags chrome://browser/content/browser.js:807:7
	loadURIWithFlags chrome://browser/content/tabbrowser.xml:7064:13
	addTab chrome://browser/content/tabbrowser.xml:2110:17
	loadOneTab chrome://browser/content/tabbrowser.xml:1523:23
	_openFirstRunPage resource://gre/modules/TelemetryReportingPolicy.jsm:462:15
	TelemetryReportingPolicyImpl.observe resource://gre/modules/TelemetryReportingPolicy.jsm:478:13
	initializeWindow resource:///modules/sessionstore/SessionStore.jsm:1050:9
	SessionStoreInternal.onBeforeBrowserWindowShown/< resource:///modules/sessionstore/SessionStore.jsm:1200:9
	Handler.prototype.process resource://gre/modules/Promise-backend.js:937:23
	this.PromiseWalker.walkerLoop resource://gre/modules/Promise-backend.js:816:7
	bound  self-hosted
	bound bound  self-hosted
	this.PromiseWalker.scheduleWalkerLoop/< resource://gre/modules/Promise-backend.js:750:11


As far as I can tell from the stack, this is because we add the listener and then load a page in a tab, where we assign to the 'tab' variable when the loadOneTab call returns. However, the listener seems to be invoked synchronously while loadOneTab hasn't returned yet, so the 'tab' variable has not been initialized yet.

I don't believe there are any ill effects from this as the conditions in the listener are guaranteed not to have been met yet anyway, and because it's just a progress listener it'll error out and just run properly the next time it's invoked, but we should probably still squash the error one way or another. Dão, can you take a look?
Flags: needinfo?(dao+bmo)
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Comment on attachment 8827841 [details]
Bug 1331873 - Null-check tab in TelemetryReportingPolicyImpl._openFirstRunPage's progress listener since it can be called synchronously before we have a reference to the tab.

https://reviewboard.mozilla.org/r/105428/#review106244
Attachment #8827841 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef886fa0b8f1
Null-check tab in TelemetryReportingPolicyImpl._openFirstRunPage's progress listener since it can be called synchronously before we have a reference to the tab. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/ef886fa0b8f1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment on attachment 8827841 [details]
Bug 1331873 - Null-check tab in TelemetryReportingPolicyImpl._openFirstRunPage's progress listener since it can be called synchronously before we have a reference to the tab.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1324049
[User impact if declined]: this code isn't currently pref'd on on Aurora, but if for some reason we decide to flip the pref in 52, it will be better to have this fixed, even though the error is handled gracefully (which is why I'm not requesting Beta/Release approval for the funnelcake)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: basically just adding a null-check
[String changes made/needed]: /
Attachment #8827841 - Flags: approval-mozilla-aurora?
Comment on attachment 8827841 [details]
Bug 1331873 - Null-check tab in TelemetryReportingPolicyImpl._openFirstRunPage's progress listener since it can be called synchronously before we have a reference to the tab.

add missing null-check, aurora52+
Attachment #8827841 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting qe-verify- based on Dão's assessment on manual testing needs (see Comment 5).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.