1608281 - Stop reading properties on the global `this` in JSMs

Status: RESOLVED FIXED

(Firefox :: General, task, P5)

Description

[Brian Grinstead [:bgrins]]

For example: Object.freeze(this.CustomizableUI); in https://searchfox.org/mozilla-central/rev/ba4fab1cc2f1c9c4e07cdb71542b8d441707c577/browser/components/customizableui/CustomizableUI.jsm#4447.

This is related to Bug 1308512 - I expect at least some of the consumers that do this are earlier setting something on this and can hopefully change to use const instead.

Comment 1

[Masatoshi Kimura [:emk]]

const won't work as you expect:

const foo = { a: 1 };
"use strict";foo.a = 2;
foo.a; // 2
var bar = { a: 1 };  // let also works
Object.freeze(bar);
"use strict";bar.a = 2; // Error

Why can't it just use Object.freeze(CustomizableUI);?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Masatoshi Kimura [:emk] from comment #1)

Why can't it just use Object.freeze(CustomizableUI);?

My script for Bug 1608278 does ultimately do that rewrite. But it would be relatively easy to split things out to make just that change here, so I'll block that bug on this change.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1608281 - Stop unintenionally reading a property off of the global `this` in FirstStartup

This code currently returns undefined:

var {FirstStartup} = ChromeUtils.import("resource://gre/modules/FirstStartup.jsm");
FirstStartup.state

But it's meant to return 0 (NOT_STARTED), and this patch fixes that.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae7ccf443295
Stop unintenionally reading a property off of the global `this` in FirstStartup r=rhelmer

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: Bug 1608281 - Automated rewrite away from reading properties on the global `this` in JSM files - round 1

This patch was generated with a script. It doesn't include all files:

• Files that use the preprocessor or fail to parse are skipped
• Files that are loaded as JSMs but don't use the .jsm extension are skipped (those will be renamed in Bug 1609269)

It was generated with the following command:

hg revert --all &&
cp .gitignore .rgignore &&
rg --files-without-match -g '*.jsm' '^#endif|^#include|^#filter' | jscodeshift --stdin --transform ~/Code/jsm-rewrites/no-this-property-read.js --ignore-pattern ./mobile/android/modules/Sanitizer.jsm --ignore-pattern ./js/xpconnect/tests/unit/syntax_error.jsm &&
./mach eslint hg st | rg '^M ' | sed 's/^M //'

Comment 6

[Oana Pop-Rus]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b17b4a20b809
Automated rewrite away from reading properties on the global `this` in JSM files - round 1 r=mossop

Comment 8

[Cristian Brindusan [:cbrindusan]]

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

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:bgrins, maybe it's time to close this bug?

Comment 11

[Brian Grinstead [:bgrins]]

Closing this one. There were still some more consumers but they can be fixed separately.