ReferenceError: can't access lexical declaration `SYSTEM_ADDON_INSTALL_DATE' before initialization at C:/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:781

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorgk, Assigned: mhowell)

Tracking

({regression})

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
First seen on Daily build of 30 Sept. 2016 here:

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=907ba80124fe9638e436f824e7f4aa11cb5198b5&selectedJob=48986

ReferenceError: can't access lexical declaration `SYSTEM_ADDON_INSTALL_DATE' before initialization at C:/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:781
(Reporter)

Comment 1

3 years ago
Aleth, can you please take a look.

I looked at the log of test_TelemetryEnvironment.js and saw bug 1292360 which landed on 30 Sept. 2016, but I can't make much sense of it (besides, I'm fighting other fires).
Flags: needinfo?(aleth)
(Reporter)

Comment 2

3 years ago
Hi Matt, looks like since bug 1292360 landed, we get test failures, see comment #0. Do you have any hints for us?
Flags: needinfo?(mhowell)
(Assignee)

Comment 3

3 years ago
Posted patch PatchSplinter Review
I should probably have realized this would happen when I changed this test. The problem is that bug 1292360 depends on bug 1277627, and that patch adds its code to /browser (which in turn runs but doesn't do anything without an installer change which is in /browser). So this test, which is in /toolkit, is now trying to load a module from /browser.

Here's a patch that breaks the dependency by checking if MOZ_BUILD_APP == 'browser' before loading the attribution module. I'm not sure if that's the ideal approach, but it seems to work.
Assignee: nobody → mhowell
Flags: needinfo?(mhowell)
Flags: needinfo?(aleth)
Attachment #8796977 - Flags: review?(gfritzsche)
(Assignee)

Updated

3 years ago
Attachment #8796977 - Attachment description: bug1306699.diff → Patch
(Reporter)

Comment 4

3 years ago
Thanks for addressing this quickly. We had another bug due toolkit relying on browser (bug 1285735).
Comment on attachment 8796977 [details] [diff] [review]
Patch

Review of attachment 8796977 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good.

However, i'm not sure how the error ("can't access lexical declaration `SYSTEM_ADDON_INSTALL_DATE'") relates to the changes here.
Can you expand on that and fix that as well if needed?
Attachment #8796977 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 6

3 years ago
Thanks. My guess is that the failure of the Cu.import() call means that initialization of the global constants (which are all later down in the file) doesn't get to happen, and SYSTEM_ADDON_INSTALL_DATE happens to be the first one of those that's accessed, so it's the one that gets the error. I did run the test locally with this patch, and it passed, so I'm confident this does really fix the problem.
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f755f8c1e472b093b053688f7431eb3e8933e4
Bug 1306699 - Only invoke attribution code functionality in desktop Firefox; r=gfritzsche
(In reply to Matt Howell [:mhowell] from comment #6)
> Thanks. My guess is that the failure of the Cu.import() call means that
> initialization of the global constants (which are all later down in the
> file) doesn't get to happen, and SYSTEM_ADDON_INSTALL_DATE happens to be the
> first one of those that's accessed, so it's the one that gets the error. I
> did run the test locally with this patch, and it passed, so I'm confident
> this does really fix the problem.

Did you also reproduce the failure without the change applied?
Flags: needinfo?(mhowell)
(Assignee)

Comment 9

3 years ago
Yes, with the same message.
Flags: needinfo?(mhowell)
Great, thanks.

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3f755f8c1e4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
(Reporter)

Updated

3 years ago
Component: General → Telemetry
Product: Thunderbird → Toolkit
Target Milestone: Thunderbird 52.0 → mozilla52
(Reporter)

Comment 12

3 years ago
This ain't no Thunderbird bug ;-)
Thanks for addressing it so quickly!

Comment 13

3 years ago
Comment on attachment 8796977 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: 1292360, 1277627
[User impact if declined]: test failures
[Risks and why]: Regressing bugs got uplifted to 50, so should this.
Attachment #8796977 - Flags: approval-mozilla-beta?
Attachment #8796977 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 14

3 years ago
Aleth: On C-B all the Xpcshell tests are broken with:
ReferenceError: FileUtils is not defined at /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:777
Return code: 1

** ReferenceError: FileUtils is not defined ... *

Is this related to this bug here or the bug 1292360 or bug 1277627?
Flags: needinfo?(aleth)

Comment 15

3 years ago
(In reply to Jorg K (GMT+2) from comment #14)
> Is this related to this bug here or the bug 1292360 or bug 1277627?

Certainly at least some of the failures are related, we'll see after this is uplifted whether it takes care of all of them.
Flags: needinfo?(aleth)
Keywords: regression
Comment on attachment 8796977 [details] [diff] [review]
Patch

Fixes a regression, Aurora51+, Beta50+
Attachment #8796977 - Flags: approval-mozilla-beta?
Attachment #8796977 - Flags: approval-mozilla-beta+
Attachment #8796977 - Flags: approval-mozilla-aurora?
Attachment #8796977 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 19

3 years ago
(In reply to aleth [:aleth] from comment #15)
> Certainly at least some of the failures are related, we'll see after this is
> uplifted whether it takes care of all of them.
I did a push to comm-beta to fix a built sync issue and it came out pretty good, so thanks to all involved.
You need to log in before you can comment on or make changes to this bug.