625440 - Error: Validation task could not complete or completed with errors

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[krupa raj[:krupa]]

Attachment: screenshot

Check the validation results @ https://addons.allizom.org/z/en-US/developers/addon/sort-tabs/file/98660/validation

All the tests failed validation with "Error: Validation task could not complete or completed with errors"

But the status message at the top says "Add-on passed validation."

Comment 1

[Kumar McMillan [:kumar]]

This is an interesting one.  The Add-on is somehow triggering 

JSReflectException: u'InternalError: too much recursion'

Traceback: http://pastebin.mozilla.org/940835

Matt, any ideas for how to work around this?  As a *last resort* the JS code could catch this exception but it would be nicer if we could discover the root cause first.

Comment 2

[Matt Basta [:basta]]

I'll need to identify exactly which part of the JS is causing this problem.

Krupa: can you attach the add-on for me to inspect?

Comment 3

[krupa raj[:krupa]]

Attachment: test file

Comment 4

[Matt Basta [:basta]]

It looks like this error is being thrown by Spidermonkey in the JSON.stringify or Reflect.parse function. There's really nothing we can do, short of either writing our own non-recursive version of JSON.stringify or skipping Spiermonkey altogether and writing a Python version of the tokenizer.

I've even considered porting the JS testcases from Python to JS and running them there to avoid using JSON.stringify, but that still wouldn't solve the problem because the JS tests use between two and three layers of recursion for every nested token in the input code, which means that the tests themselves would throw a recursion error.

Perhaps there's a way to eliminate the recursion cap? Otherwise, I'd say the best solution is to throw some kind of warning for the editors and skip the file.

Comment 5

[Kumar McMillan [:kumar]]

What about just catching 'InternalError: too much recursion' and displaying an informative error to the developer?  Currently, all they see is "could not complete validation" which doesn't give them a chance to fix the problem.  I'm not sure *how* they would fix it but maybe by splitting up their JS into separate files.

Comment 6

[Matt Basta [:basta]]

I would imagine that the solution wouldn't be to split up their JS, but rather to decrease the depth of which they nest their code. It'd be a whole restructuring task.

For now, I'll make this into a notice. At least then it can be used as an indicator to the editors that a file wasn't validated.

Comment 7

[Wil Clouser [:clouserw]]

Making it an informational notice is good for now.

Comment 8

[Matt Basta [:basta]]

Done:

https://github.com/mattbasta/amo-validator/commit/1d35e959a586af7fd41277a2899da208a26e2bd4

Comment 9

[Matt Basta [:basta]]

Sorry, wrong commit:

https://github.com/mattbasta/amo-validator/commit/d51b615a0db21f58bde77cde4df2aacef6084e7e

Comment 10

[Kumar McMillan [:kumar]]

Pushed to Zamboni in https://github.com/jbalogh/zamboni-lib/commit/b2495e1