934388 - Add-on shouldn't fail validation with deprecated //@ pragma

Status: VERIFIED FIXED

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

Description

[Dimitriy]

Attachment: jquery.min.js

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Steps to reproduce:

I used official jQuery from officail web-site, but Mozilla developers didn't approved my extension because of this error.


Actual results:

 An error in the JavaScript file prevented it from being properly read by the Spidermonkey JS engine.

'1:0 warning: Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead\n'


Expected results:

File should be read.

Comment 1

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

I think Validation is a better component for this than the Builder.

Comment 2

[Matt Basta [:basta]]

Would you please attach your full extension?

Comment 3

[Rob Campbell [:rc] (:robcee)]

This is because jquery is still using the old style //@ sourcemapurl pragma. They need to switch to //#.

Comment 4

[Matt Basta [:basta]]

We shouldn't fail validation because of this, though. Ideally we'd raise a warning. There should be a way to have Spidermonkey ignore the old-style sourcemapurl pragma. It might be safer to match and replace that with a regex, raising a warning if it's found. I don't want to implement a brittle solution, though.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

SpiderMonkey is giving a warning, as expected. Not sure why AMO reviewers consider this a blocker for accepting an addon, though.

Comment 6

[Matt Basta [:basta]]

They aren't. Since Spidermonkey returns the warning on stderr, we consider it a syntax error (there's no other kind of error that should be returned) and the automated tool rejects the add-on before the reviewers ever see it.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I suppose once it is classified as a proper warning this should be fixed? Bug 926595.

Comment 8

[Matt Basta [:basta]]

That'd work as well, but doesn't provide a short-term solution. Really, warnings shouldn't be generated when using the reflection API, IMO. It either parses or it doesn't, yeah? I suppose it could go either way.

Comment 10

[Mark Striemer [:mstriemer]]

The current workaround should be to submit a non-minified version of jquery. Waiting on a fix in SpiderMonkey.

Comment 11

[Kris Maglione [:kmag]]

This is causing people to upload modified versions of libraries that we wind up having to reject. It needs to be fixed ASAP.

The simplest solution would be to run `js -W`. The more sensible one would probably be to parse the output, or simply treat stderr output as a warning rather than a fatal error.

Comment 12

[Yohan Boniface [:ybon]]

Any idea what could be the delay of a proper fix on the SpiderMonkey side? I wonder if we need to spend time on a temporary fix if a clean one is on its way.

Comment 13

[Kris Maglione [:kmag]]

I don't know what's delaying the fix in SpiderMonkey, but I do think that this needs to be fixed in the validator. Regardless, I don't think that we should be throwing an error and completely ignoring a JS file just because there's stderr output. Turning this into a warning and actually processing the JS file is a trivial fix.

Comment 14

[Yohan Boniface [:ybon]]

We just run some tests with Mark [:mstriemer], and we found out that the given jquery file passes with no error nor stderr in local.
We checked out the version and compared with our servers, and we have more recent version in local (JavaScript-C31.0a1 for me, instead of JavaScript-C27.0a1 in our servers).

Plan now is to see if we can just update it. We'll ask Krupa when she's online how we can QA such update.

Comment 15

[Yohan Boniface [:ybon]]

Just discussed with krupa about updating SpiderMonkey version in our servers.

She noticed me that this is a lot of thing to review. Given that we already have the move to contrib.staticfiles in the pipe, she warned that it should be better not to push two big changes. And I must agree that this should not be reasonable.

I see that this ticket has been set as "blocker" but contrib.staticfiles is already merged, so the situation is a bit tricky. Maybe we can do a "SpiderMonkey" dedicated session the week after next week?

Kris, Wil, what are you thoughts on this?

Comment 16

[Kris Maglione [:kmag]]

If updating SpiderMonkey fixes this, then we should do that as soon as possible. If you don't think it's a good idea for this push, I'm fine with that, but this still needs to be fixed.

Either way, I think that we should not be raising a validation error if there's stderr output from SpiderMonkey. Even for parse errors, we only raise a warning. For stderr, we should probably convert the output to warning text, and try to parse the stdout output anyway.

Comment 17

[Wil Clouser [:clouserw]]

We talked on IRC about this and I thought we were just making the spidermonkey output a warning instead of an error.  That doesn't seem like a big change.  Can we fit that in to this push?

Comment 18

[Yohan Boniface [:ybon]]

Sure, but in the meantime we found out that upgrading SpiderMonkey does fix the issue, this is why I was waiting for your opinion.
That said, the message is quite clear, so I'll work on this tomorrow.

Comment 19

[Mark Striemer [:mstriemer]]

Attachment: @pragma-error.xpi

This addon includes the jquery.min.js file that is throwing an error on production. Works on -dev.

Comment 20

[David Larlet [:davidbgk]]

Closing it since bug 1050848 has been closed.

Comment 21

[Kris Maglione [:kmag]]

QA, please test this on production with the add-on in comment 19.

Comment 22

[Victor Carciu]

Verified on -dev and I was able to submit .

Comment 23

[Victor Carciu]

Sorry, verified in production per comment #21. Everything is working as expected.