I think Validation is a better component for this than the Builder.
Would you please attach your full extension?
This is because jquery is still using the old style //@ sourcemapurl pragma. They need to switch to //#.
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.
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.
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.
The current workaround should be to submit a non-minified version of jquery. Waiting on a fix in SpiderMonkey.
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.
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.
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?
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.
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?
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.
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.
Closing it since bug 1050848 has been closed.
QA, please test this on production with the add-on in comment 19.
Verified on -dev and I was able to submit .
Sorry, verified in production per comment #21. Everything is working as expected.