New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid metadata in "New script template" prevents creation of new scripts #2336

Closed
NoneGiven opened this Issue Nov 15, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@NoneGiven
Contributor

NoneGiven commented Nov 15, 2015

I ran into this issue because mysteriously, months after I last touched this setting, the newlines in my custom template were lost/replaced.

To reproduce, put some random junk in "New script template" in Greasemonkey's options panel, go to add a new script, and click the Okay button to add it. Observe the null reference exception in the console and the lack of a new script being added.

The variable meta in parseScript.js::parse() coming from extractMeta.js::extractMeta() is null after attempting to extract invalid metadata (even though extractMeta() is clearly supposed to return an empty string if the regex doesn't match). There are a couple of "if not meta" lines in parse() that look like they should fall back on defaults for a null/empty metadata string, but apparently aNoMetaOk is true so it keeps going until the null reference.

I would have submitted a PR but I'm not sure what exactly aNoMetaOk's purpose in life is and what changes are okay there.

@janekptacijarabaci

This comment has been minimized.

Show comment
Hide comment
@janekptacijarabaci

janekptacijarabaci Nov 15, 2015

Contributor

See also (meta is null) - after script editing:

// ==UserScript==
// ==/UserScript==

var something;

A little strange code (AFAIK):
https://github.com/greasemonkey/greasemonkey/blob/3.5/modules/parseScript.js#L35
vs
https://github.com/greasemonkey/greasemonkey/blob/3.5/modules/parseScript.js#L178


Ad https://github.com/greasemonkey/greasemonkey/blob/3.5/modules/parseScript.js#L35:

Option 1:

From:

if (!meta && aNoMetaOk) {
  setDefaults(script);
  return script;
}

var resourceNames = {};
for (var i = 0, metaLine = ''; metaLine = meta[i]; i++) {

To:
(Option 1a)

var resourceNames = {};
for (var i = 0, count = meta ? meta.length : 0; i < count; i++) {
  var metaLine = meta[i];

or
(Option 1b)

if (!meta) {
  if (aNoMetaOk) {
    setDefaults(script);
    return script;
  } else {
    // log error... (script.parseErrors.push...)
  }
}

var resourceNames = {};
for (var i = 0, count = meta ? meta.length : 0; i < count; i++) {
  var metaLine = meta[i];

Option 2:
"All" function calls... (scope.parse() => aNoMetaOk[true = if true]) + "Option 1b (log error)"

?

Contributor

janekptacijarabaci commented Nov 15, 2015

See also (meta is null) - after script editing:

// ==UserScript==
// ==/UserScript==

var something;

A little strange code (AFAIK):
https://github.com/greasemonkey/greasemonkey/blob/3.5/modules/parseScript.js#L35
vs
https://github.com/greasemonkey/greasemonkey/blob/3.5/modules/parseScript.js#L178


Ad https://github.com/greasemonkey/greasemonkey/blob/3.5/modules/parseScript.js#L35:

Option 1:

From:

if (!meta && aNoMetaOk) {
  setDefaults(script);
  return script;
}

var resourceNames = {};
for (var i = 0, metaLine = ''; metaLine = meta[i]; i++) {

To:
(Option 1a)

var resourceNames = {};
for (var i = 0, count = meta ? meta.length : 0; i < count; i++) {
  var metaLine = meta[i];

or
(Option 1b)

if (!meta) {
  if (aNoMetaOk) {
    setDefaults(script);
    return script;
  } else {
    // log error... (script.parseErrors.push...)
  }
}

var resourceNames = {};
for (var i = 0, count = meta ? meta.length : 0; i < count; i++) {
  var metaLine = meta[i];

Option 2:
"All" function calls... (scope.parse() => aNoMetaOk[true = if true]) + "Option 1b (log error)"

?

@janekptacijarabaci

This comment has been minimized.

Show comment
Hide comment

@arantius arantius added this to the 3.7 milestone Jan 20, 2016

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Jan 20, 2016

Collaborator

Ok, I opened th GM Options dialog, typed "foo" into the template box, hit OK. Then Monkey menu>new user script. Type any name and hit OK. Get an error in the console with traceback:

12:12:47.004 TypeError: meta is null
parse() parseScript.js:41
doInstall() newscript.js:52
anonymous() dialog.xml line 386 > Function:1
_fireButtonEvent() dialog.xml:387
_doButtonCommand() dialog.xml:355
_handleButtonCommand() dialog.xml:343
1 parseScript.js:41:34

The line parseScript.js:41 is trying to loop over meta, which is null. This is below the "if no meta, assume defaults and return" section, so aNoMetaOk must have been false. The line newscript.js:52 called parse(), passing no value for aNoMetaOk so it is undefined (i.e. falsy). It should have passed true. Doing so causes a failure at the attempt to install this script, which also tries to parse it, and also does not pass any aNoMetaOk value.

In fact, only one code path, RemoteScript.prototype.parseScript does pass any defined value, for either aFailWhenMissing or aNoMetaOk. What it wants is the opportunity to force a failure, though default behavior is to allow a missing/broken meta block.

All that logic is confused and should be cleaned up, which may implicitly fix this issue.

Collaborator

arantius commented Jan 20, 2016

Ok, I opened th GM Options dialog, typed "foo" into the template box, hit OK. Then Monkey menu>new user script. Type any name and hit OK. Get an error in the console with traceback:

12:12:47.004 TypeError: meta is null
parse() parseScript.js:41
doInstall() newscript.js:52
anonymous() dialog.xml line 386 > Function:1
_fireButtonEvent() dialog.xml:387
_doButtonCommand() dialog.xml:355
_handleButtonCommand() dialog.xml:343
1 parseScript.js:41:34

The line parseScript.js:41 is trying to loop over meta, which is null. This is below the "if no meta, assume defaults and return" section, so aNoMetaOk must have been false. The line newscript.js:52 called parse(), passing no value for aNoMetaOk so it is undefined (i.e. falsy). It should have passed true. Doing so causes a failure at the attempt to install this script, which also tries to parse it, and also does not pass any aNoMetaOk value.

In fact, only one code path, RemoteScript.prototype.parseScript does pass any defined value, for either aFailWhenMissing or aNoMetaOk. What it wants is the opportunity to force a failure, though default behavior is to allow a missing/broken meta block.

All that logic is confused and should be cleaned up, which may implicitly fix this issue.

@arantius arantius closed this in 9adfced Jan 20, 2016

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Jan 20, 2016

Collaborator

This should be fixed in 3.7beta2: https://addons.mozilla.org/firefox/downloads/file/388504/greasemonkey-3.7beta2-fx.xpi?src=devhub

I'd appreciate confirmation either way.

Collaborator

arantius commented Jan 20, 2016

This should be fixed in 3.7beta2: https://addons.mozilla.org/firefox/downloads/file/388504/greasemonkey-3.7beta2-fx.xpi?src=devhub

I'd appreciate confirmation either way.

@NoneGiven

This comment has been minimized.

Show comment
Hide comment
@NoneGiven

NoneGiven Jan 24, 2016

Contributor

Is there a pref or something to let me install that in Dev or Release, or does it have to be Nightly? AMO is in the list of sites allowed to install extensions but both the aforementioned versions of Firefox just inform me they're not letting AMO ask to install it, and don't give the option to allow.

Contributor

NoneGiven commented Jan 24, 2016

Is there a pref or something to let me install that in Dev or Release, or does it have to be Nightly? AMO is in the list of sites allowed to install extensions but both the aforementioned versions of Firefox just inform me they're not letting AMO ask to install it, and don't give the option to allow.

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Jan 24, 2016

Collaborator

Extensions hosted on AMO should be installable for any Firefox. Perhaps use the versions page?

https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/versions/

Collaborator

arantius commented Jan 24, 2016

Extensions hosted on AMO should be installable for any Firefox. Perhaps use the versions page?

https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/versions/

@NoneGiven

This comment has been minimized.

Show comment
Hide comment
@NoneGiven

NoneGiven Jan 25, 2016

Contributor

Thanks, I guess it was just the direct link it didn't like. Can confirm this issue is fixed.

Contributor

NoneGiven commented Jan 25, 2016

Thanks, I guess it was just the direct link it didn't like. Can confirm this issue is fixed.

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Jan 25, 2016

Collaborator

I think Firefox controls "can install an XPI" by the site that the link is on, rather than the site that the XPI is served from.

Collaborator

arantius commented Jan 25, 2016

I think Firefox controls "can install an XPI" by the site that the link is on, rather than the site that the XPI is served from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment