634381 - Add-on validator should check for synchronous XMLHttpRequests (XHRs) and explain why they are bad

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, enhancement, P3)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The add-on validator should check for synchronous XMLHttpRequests (XHRs) and explain why they are bad.

Comment 1

[Matt Basta [:basta]]

Could you be more specific? Feel free to post scenarios to test for and wording for such a warning.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Synchronous XHRs block the application UI if they run on the main thread (which is often the case), see https://developer.mozilla.org/en/using_xmlhttprequest#Example.3a_Asynchronous_request

So the validator should check for all XHRs with the third attribute as "false". There are some extensions reading local files like this, these should probably be excluded. 
I am sorry that I am not common with regular expressions, but the filters should look somehow like this:
\(\s*\"GET\".*false\s*\)

Comment 3

[Wil Clouser [:clouserw]]

Jorge, is this something we want?  What wording should we use?

Assuming we add this, I'd rather not use a fragile regex.  Can we do this with spidermonkey instead?

Comment 4

[Matt Basta [:basta]]

As far as accuracy goes, the validator can tell what type a variable is derived from (so long as the type is not declared locally), so I can test to see if an XMLHttpRequest object has its open() function called with a falsey value for the third parameter. The accuracy would be on par with the other multi-step JS tests.

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

Warning message:
Synchronous HTTP requests can cause serious UI performance problems, specially to users with slow network connections.

It should be possible to use SpiderMonkey. One note: there are 2 ways to create an XMLHttpRequest object. See https://developer.mozilla.org/en/using_xmlhttprequest#Using_XMLHttpRequest_from_JavaScript_modules_.2f_XPCOM.c2.a0components

Comment 6

[Matt Basta [:basta]]

The standard XMLHttpRequest update should be relatively simple. The XPCOM version will be pretty tricky, though. I'll work on getting the standard one nailed down first, as that seems like it's the most pressing.

Comment 7

[Matt Basta [:basta]]

Done (for XMLHttpRequest objects):

https://github.com/mattbasta/amo-validator/commit/87f6437c3564b5a76fafb931378bcfc456e1f5a8

How much of a priority is the XPCOM way?

Comment 8

[Jorge Villalobos [:jorgev] (he/him)]

I don't have any numbers, but I think there are more uses of the XPCOM way than the other one. At any rate I think we should get both for consistency. This is not a very high priority, but I'd rather see both implemented.

Comment 9

[Matt Basta [:basta]]

Done and implemented:

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

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...