If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in 2014-10

Status

addons.mozilla.org Graveyard
Add-on Validation
P1
blocker
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Dimitriy, Unassigned)

Tracking

unspecified
2014-10
Dependency tree / graph

Details

(Whiteboard: [sourcemaps][ReviewTeam:P1])

Attachments

(2 attachments)

81.55 KB, application/javascript
Details
35.72 KB, application/x-xpinstall
Details
(Reporter)

Description

4 years ago
Created attachment 826658 [details]
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.
I think Validation is a better component for this than the Builder.
Component: Add-on Builder → Add-on Validation

Comment 2

4 years ago
Would you please attach your full extension?
Flags: needinfo?(light.it.zp)
This is because jquery is still using the old style //@ sourcemapurl pragma. They need to switch to //#.
Whiteboard: [sourcemaps]

Comment 4

4 years ago
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.
SpiderMonkey is giving a warning, as expected. Not sure why AMO reviewers consider this a blocker for accepting an addon, though.

Comment 6

4 years ago
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.
I suppose once it is classified as a proper warning this should be fixed? Bug 926595.
Depends on: 926595

Comment 8

4 years ago
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.

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Error: 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' → Add-on shouldn't fail validation with Error: 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'
Duplicate of this bug: 952417
The current workaround should be to submit a non-minified version of jquery. Waiting on a fix in SpiderMonkey.
Priority: -- → P2
Summary: Add-on shouldn't fail validation with Error: 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' → Add-on shouldn't fail validation with deprecated //@ pragma
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.
Severity: normal → major
Severity: major → blocker
Whiteboard: [sourcemaps] → [sourcemaps][ReviewTeam:P1]
Target Milestone: --- → 2014-05
Priority: P2 → P1
Target Milestone: 2014-05 → 2014-08
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.
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.
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.
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?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(clouserw)
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.
Flags: needinfo?(kmaglione+bmo)
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?
Flags: needinfo?(clouserw)
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.

Updated

3 years ago
Depends on: 1050848
Created attachment 8477066 [details]
@pragma-error.xpi

This addon includes the jquery.min.js file that is throwing an error on production. Works on -dev.
Target Milestone: 2014-08 → 2014-09
Target Milestone: 2014-09 → 2014-10
Closing it since bug 1050848 has been closed.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(light.it.zp)
Resolution: --- → FIXED
QA, please test this on production with the add-on in comment 19.

Comment 22

3 years ago
Verified on -dev and I was able to submit .
Status: RESOLVED → VERIFIED

Comment 23

3 years ago
Sorry, verified in production per comment #21. Everything is working as expected.
(Assignee)

Updated

2 years ago
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.