1331873 - First run page for data choices causes JS error

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (he/him)]

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?

Comment 1

[Dão Gottwald [:dao]]

Attachment: 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.

Review commit: https://reviewboard.mozilla.org/r/105428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105428/

Comment 2

[:Gijs (he/him)]

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
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

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef886fa0b8f1

Comment 5

[Dão Gottwald [:dao]]

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]: /

Comment 6

[Julien Cristau [:jcristau]]

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+

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd4fc47221cb

Comment 8

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Dão's assessment on manual testing needs (see Comment 5).