1185355 - Compile-time errors in `contentScript` values should be emitted as warnings

Status: RESOLVED INCOMPLETE

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

Description

[Mukesh]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Steps to reproduce:

Code in main.js:
var com = 'some_codes'+var + ';';

    tabs.activeTab.attach({
        contentScript: com
    });

Error in https://addons.mozilla.org/en-us/developers/addon/validate:
compile-time error in the JavaScript halted validation of that file.  Message: expected expression, got '; 


Actual results:

Above addon work correctly but https://addons.mozilla.org/en-us/developers/addon/validate gave warning(message)


Expected results:

above addon work correctly

Comment 1

[Mathieu Agopian [:magopian]]

Kris, is that "expected"?

Comment 2

[Mukesh]

Expected results:

above addon work correctly

https://addons.mozilla.org/en-us/developers/addon/validate:
should have gave error

Comment 3

[Mukesh]

Edited
Expected results:

above addon work correctly

https://addons.mozilla.org/en-us/developers/addon/validate:
should not have gave error

Comment 4

[Mukesh]

(In reply to Mathieu Agopian [:magopian] from comment #1)
> Kris, is that "expected"?

Edited
Expected results:

above addon work correctly

https://addons.mozilla.org/en-us/developers/addon/validate:
should not have gave error

Comment 5

[Mukesh]

my addons was not accepted due to this but was accepted on 2nd try (i deleted unused file on 2nd try but unused file was not the reason why it was reject in 1st try)

stackoverflow link below has screenshot

Here is my addon:

https://addons.mozilla.org/en-US/firefox/addon/youtubevideospeedcontroller/?src=search

I posted the question on http://stackoverflow.com(Any solution or i just ignore warning):
http://stackoverflow.com/questions/31502058/addons-mozilla-org-validation-shows-compile-time-error-in-the-javascript

Thank you!!

Comment 6

[Kris Maglione [:kmag]]

Ah, I see, by `var`, you mean a variable name, and not the reserved word.

It looks like what's happening is the `speed` variable is being treated as if it were an empty string, which means that the resulting script, `document.getElementsByTagName("video")[0].playbackRate=;` is not valid.

This is something that we need to fix, but your code itself also needs to be fixed. We don't accept dynamically-generated strings as `contentScript` values. You should ideally be using an external file, with `contentScriptFile` rather than `contentScript`, and should be passing the `speed` value via `contentScriptOptions` (see [1] for more information). It would be acceptable to pass a static string to the `contentScript` property, in this case, given the simplicity of the code fragment. Just make sure you're still passing any values via `contentScriptOptions`.


[1] https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts/Loading_Content_Scripts

Comment 7

[Mukesh]

(In reply to Kris Maglione [:kmag] from comment #6)
> Ah, I see, by `var`, you mean a variable name, and not the reserved word.
> 
> It looks like what's happening is the `speed` variable is being treated as
> if it were an empty string, which means that the resulting script,
> `document.getElementsByTagName("video")[0].playbackRate=;` is not valid.
> 
> This is something that we need to fix, but your code itself also needs to be
> fixed. We don't accept dynamically-generated strings as `contentScript`
> values. You should ideally be using an external file, with
> `contentScriptFile` rather than `contentScript`, and should be passing the
> `speed` value via `contentScriptOptions` (see [1] for more information). It
> would be acceptable to pass a static string to the `contentScript` property,
> in this case, given the simplicity of the code fragment. Just make sure
> you're still passing any values via `contentScriptOptions`.
> 
> 
> [1]
> https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts/
> Loading_Content_Scripts

that is what i was thinking But speed is never empty.Is it?my addon is working and i put log to check empty string but speed is never empty.

Moreover, Message says: expected expression,  got';  it was not generated by empty string

So i have to use contentScriptFile,but still is not it giving wrong message,var com should have acts as static string(even if it is generated dynamically)

Comment 8

[Mukesh]

(In reply to mukeshdevnp from comment #7)
> (In reply to Kris Maglione [:kmag] from comment #6)
> > Ah, I see, by `var`, you mean a variable name, and not the reserved word.
> > 
> > It looks like what's happening is the `speed` variable is being treated as
> > if it were an empty string, which means that the resulting script,
> > `document.getElementsByTagName("video")[0].playbackRate=;` is not valid.
> > 
> > This is something that we need to fix, but your code itself also needs to be
> > fixed. We don't accept dynamically-generated strings as `contentScript`
> > values. You should ideally be using an external file, with
> > `contentScriptFile` rather than `contentScript`, and should be passing the
> > `speed` value via `contentScriptOptions` (see [1] for more information). It
> > would be acceptable to pass a static string to the `contentScript` property,
> > in this case, given the simplicity of the code fragment. Just make sure
> > you're still passing any values via `contentScriptOptions`.
> > 
> > 
> > [1]
> > https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts/
> > Loading_Content_Scripts
> 
> that is what i was thinking But speed is never empty.Is it?my addon is
> working and i put log to check empty string but speed is never empty.
> 
> Moreover, Message says: expected expression,  got';  it was not generated by
> empty string
> 
> So i have to use contentScriptFile,but still is not it giving wrong
> message,var com should have acts as static string(even if it is generated
> dynamically)

Ok thanks

https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts/Loading_Content_Scripts

"Unless your content script is extremely simple and consists only of a static string, don't use contentScript: if you do, you may have problems getting your add-on approved on AMO.

Instead, keep the script in a separate file and load it using contentScriptFile. This makes your code easier to maintain, secure, debug and review"

Comment 9

[Kris Maglione [:kmag]]

(In reply to mukeshdevnp from comment #7)
> that is what i was thinking But speed is never empty.Is it?my addon is
> working and i put log to check empty string but speed is never empty.

The validator does not know what values `speed` has. It doesn't run the code, and it can't trace function calls in this kind of detail, so it treats `speed` as an empty string.

> Moreover, Message says: expected expression,  got';  it was not generated by
> empty string

The final value of `com`, when `speed` is treated as an empty string, ends with `=;`, which generates that syntax error.

> So i have to use contentScriptFile,but still is not it giving wrong
> message,var com should have acts as static string(even if it is generated
> dynamically)

I agree that the message is wrong, in that `speed` should be treated as an unknown, and should not have resulted in a string that the validator thought it could test. It should have resulted in a warning that dynamic values should never be used as `contentScript` values. But that's a more complicated issue, which I don't think will be fixed in the short term.

Comment 11

[Mukesh]

(In reply to Kris Maglione [:kmag] from comment #9)

I am new here

Ok,so i have to fix my code but what happens to this bug will someone fix it or it is not a bug


my addon was not accepted due to this warning at first(it was accepted later) so will addon be accepted if this warning is contain in any addon

Comment 12

[Mukesh]

I got mail from addons support that it was a bug on their side and my program was correct(no need to fix my code) 

so will the bug be fix ???What happens now

Comment 13

[Nootan Ghimire]

Haven't checked but it seems that adding this line just after function definition:

speed_html = speed_html || 1; 

should result in correct validation as there is no chance that speed_html will be empty ever?

Comment 14

[Nootan Ghimire]

or:

speed_html = speed_html || 1;

if(speed_html.trim() == "") speed_html = 1;

Comment 15

[Mukesh]

worked !

Comment 16

[Kris Maglione [:kmag]]

Neither of those is a correct solution, and either of them should result in your add-on being rejected. If you are using `contentScript` properties, they need to be static strings.

Comment 17

[Mukesh]

if(speed.trim() == "") speed = 1;

I mean it by pass the warning message, "" has been replaced with 1 as default value.
yes contentScriptFile is the better solution

the bug is still there ,showing wrong warning message