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

RESOLVED FIXED in 6.0.0

Status

addons.mozilla.org Graveyard
Admin/Editor Tools
P3
enhancement
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: aryx, Assigned: basta)

Tracking

Details

(Whiteboard: [ReviewTeam])

The add-on validator should check for synchronous XMLHttpRequests (XHRs) and explain why they are bad.
Assignee: nobody → mbasta
Whiteboard: [required amo-editors]
Target Milestone: --- → 5.12.12
(Assignee)

Comment 1

7 years ago
Could you be more specific? Feel free to post scenarios to test for and wording for such a warning.
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*\)
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?
(Assignee)

Comment 4

7 years ago
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.
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
(Assignee)

Comment 6

7 years ago
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.
(Assignee)

Comment 7

7 years ago
Done (for XMLHttpRequest objects):

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

How much of a priority is the XPCOM way?
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.
Priority: -- → P3
Target Milestone: 5.12.12 → 6.0.0
(Assignee)

Comment 9

7 years ago
Done and implemented:

https://github.com/mattbasta/amo-validator/commit/de79fcca2496899d2ea310f1096537e2ce4c70f1
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.