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

Is importing an empty file supported? #762

Closed
MeoMix opened this issue Sep 5, 2015 · 5 comments
Closed

Is importing an empty file supported? #762

MeoMix opened this issue Sep 5, 2015 · 5 comments

Comments

@MeoMix
Copy link
Contributor

MeoMix commented Sep 5, 2015

Hey,

I encountered a rather nasty bug this evening. I attempted to import a file which had only commented out lines within it. Attempting to do so caused my browser to become unresponsive. Looking at the 'Network' tab in Chrome Dev Tools I can see that the last request to successfully complete is the file in question. All XHRs after that point remain pending.

Adding anything of substance to the file and everything went back to working.

Chrome Version 45.0.2454.85 m.

Is this a known issue? Weird edge case? Something corrupt on my end?

@guybedford
Copy link
Member

Thanks for posting. Can you share an isolated replication or steps to replicate for this?

@MeoMix
Copy link
Contributor Author

MeoMix commented Sep 5, 2015

https://github.com/MeoMix/jspmdebug

I built a repo with an example inside of it.

Check it out, run the standard npm install jspm jspm init and accept all defaults.

I was testing this in a Chrome extension environment, so for me I would then:

  • go to chrome://extensions
  • check 'developer mode' checkbox
  • click 'load unpacked extension' and select parent directory of manifest.json
  • navigate to chrome-extension://jbnkffmindojffecdhbbmekbmkkfpmjd/test.html

Observe that page is totally unresponsive. If you open developer tools before navigating to that link then you can see all the network requests go and then it just hangs after trying to load test.js

If I delete all the commented out lines (or even just some of them!) from test.js then everything runs as expected.

@guybedford
Copy link
Member

Thanks for posting this.

I've managed to trace it down that the following regular expression will take a very long time:

var a = "//// /collection/\n//import 'test/background/collection/clientErrors.spec';\n//import 'test/background/collection/playlistItems.spec';\n//import 'test/background/collection/playlists.spec';\n//import 'test/background/collection/searchResults.spec';\n//import 'test/background/collection/videos.spec';\n//import 'test/background/collection/streamItems.spec';\n\n//// /model/\n//import 'test/background/model/activePlaylistManager.spec';\n//import 'test/background/model/clientErrorManager.spec';\n//import 'test/background/model/dataSource.spec';\n//import 'test/background/model/playlistItem.spec';\n//import 'test/background/model/playlistItems.spec';\n//import 'test/background/model/relatedVideosManager.spec';\n//import 'test/background/model/signInManager.spec';\n//import 'test/background/model/user.spec';\n//import 'test/background/model/youTubeV3API.spec';"

a.match(/^\s*(\s*\/\/[^\n]*|\s*"[^"]+"\s*;?|\s*'[^']+'\s*;?|\/\*[^\*]*(\*(?!\/)[^\*]*)*\*\/)*\s*System\.register(Dynamic)?\s*\(/)

I didn't think there was any bad backtracking going on here, but apparently there must be.

Will have to debug this further to correct the regex.

@guybedford
Copy link
Member

NB this should be ported for 0.18 before the 0.19 release.

@guybedford
Copy link
Member

Released in 0.19.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants