Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsdom gets stuck at traverse::end #485

Closed
qzhou1607-zz opened this issue Sep 1, 2017 · 8 comments
Closed

jsdom gets stuck at traverse::end #485

qzhou1607-zz opened this issue Sep 1, 2017 · 8 comments

Comments

@qzhou1607-zz
Copy link
Contributor

connector: jsdom
url: www.facebook.com

This happens when axe is activated and is back to normal when axe is deactivated. This delay seems to be a extra long time taken in the line below:
https://github.com/sonarwhal/sonar/blob/79b8da5d337ef915fe2895d424872bdd21ef4111/src/lib/connectors/jsdom/jsdom.ts#L467

@qzhou1607-zz
Copy link
Contributor Author

qzhou1607-zz commented Sep 1, 2017

This issue is relevant to #192. Currently we always have to wait for 60s if something is wrong with running the script.

@molant
Copy link
Member

molant commented Sep 2, 2017

if something is wrong with running the script.

Do we know if something is wrong with the script or is it just that it takes too long to execute?

@molant
Copy link
Member

molant commented Sep 2, 2017

One option could be to split the execution in multiple batches. Not sure how that would work though.

@qzhou1607-zz
Copy link
Contributor Author

Yes, below is the error I got when I paste the script to the console in www.facebook.com. So it ended up waiting for the 60s timeout in evaluate of jsdom when running axe. Plus the spinner stops spinning during this 60s (shown as \ Traversing finished). So from a user's perspective, it looks like it's stuck. Does the timeout in evaluate have to be 60s in jsdom? Can this waiting time be shorter? @molant @sarvaje

console-error

Do we know if something is wrong with the script or is it just that it takes too long to execute?

@molant
Copy link
Member

molant commented Sep 5, 2017

Does the timeout in evaluate have to be 60s in jsdom? Can this waiting time be shorter?

I think a better solution will be to create another process with just jsdom to inject the JS, maybe one process per JS evaluated so there aren't any conflicts between them. We will have to change the guidance and evaluate as soon as it makes sense (targetfetch::end?). And also maybe a custom timeout for this type of rules. This should give us better performance and in the worst case the spinner will not be blocked. @sonarwhal/contributors what do you think?

@sarvaje has been working with child process in https://github.com/sonarwhal/online-service so he can probably help you if we decide to go down this road.

@qzhou1607-zz
Copy link
Contributor Author

@molant I tried creating a child process for evaluate. However I can't figure out a way to pass in the context to the child process. I have tried so far:

  1. Passing in this._window.document, it loses some properties after serialization.
  2. Passing in this._window, it contains circular references and can't be properly serialized
  3. Passing in jsdomutils.implForWrapper(this._window.document)._global as a whole using json-circular. It then throws an error TypeError: sandbox argument must have been converted to a context.

@molant
Copy link
Member

molant commented Sep 6, 2017

@qzhou1607 the child process will have to have it's own code that only runs jsdom, navigates to that url and then evaluates the script. We should only have to send the script we want to run and then return the result of the execution if it can be serialized (and find a way if it can't be).

@sarvaje can you work with @qzhou1607? This should be somehow similar to what you are doing with the scanner.

@sarvaje
Copy link
Contributor

sarvaje commented Sep 6, 2017

ok

qzhou1607-zz added a commit to qzhou1607-zz/Sonar that referenced this issue Sep 6, 2017
qzhou1607-zz added a commit to qzhou1607-zz/Sonar that referenced this issue Sep 6, 2017
qzhou1607-zz added a commit to qzhou1607-zz/Sonar that referenced this issue Sep 7, 2017
qzhou1607-zz added a commit to qzhou1607-zz/Sonar that referenced this issue Sep 7, 2017
@alrra alrra closed this as completed in a5b9951 Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants