Skip to content
New issue

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

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

Already on GitHub? # to your account

Cesium Viewer crashes on MacOS (Safari) and iOS (all) with rapid mouse movement #5691

Closed
mramato opened this issue Jul 28, 2017 · 5 comments
Closed
Labels
good first issue An opportunity for first time contributors type - bug

Comments

@mramato
Copy link
Contributor

mramato commented Jul 28, 2017

The Viewer reference app uses replaceState pretty much every time the camera moves, this leads to a security error on all Safari versions and results in a crash. Just move them camera a whole bunch and you'll crash. Basically, you aren't allowed to call replaceState or otherwise much with history more than 100 times over 30 seconds.

While Apple having a check like this is questionable, we probably shouldn't be calling it that often to begin with.

NOTE: This only applies to the CesiumViewer reference app, which updates the URL to match the camera position, not the Viewer widget.

@mramato mramato added good first issue An opportunity for first time contributors type - bug labels Jul 28, 2017
avalan4e57 added a commit to avalan4e57/cesium that referenced this issue Jul 31, 2017
The issue is about fixing safari crash on mouse move because of using
replaceState to often.

There was a syntax error in using setTimeout function where interval
value was on the third place in func parameters list when it should
be on second.
@avalan4e57
Copy link

Hello. I'm new to contributing on open source projects. Don't know if I got you exactly, but there's a pull request with how I see the fix of the problem. By the way I can't test the stuff locally because I get this type of errors in browser js console error for: Cesium/Shaders/.... A 404 one if to be exact. Can you please help me with that?

@hpinkos
Copy link
Contributor

hpinkos commented Jul 31, 2017

Thanks for looking into the problem @avalan4e57! For running the code locally, see the information in our Build Guide. When you're ready to open a pull request, take a look at our contributing documentation for guidelines.

I saw your commit and I think that's a good start (that code is clearly incorrect and you're change fixes that), but I think we can make it even better by listening to the Camera.changed event instead of moveStart/moveEnd, and using a setTimeout/clearTimeout instead of a setInterval to make sure replaceState is only called at most once a second.

And yes, the camera parameter passed to the setInterval function looks like it can be removed

avalan4e57 added a commit to avalan4e57/cesium that referenced this issue Jul 31, 2017
Some fixes to previous commit as @hpinkos recommended

Replaced eventListener moveStart / moveEnd with Camera.changed
event listener. Replaced setInterval with setTimeout function in
that event.
@avalan4e57
Copy link

Thanks for reply @hpinkos ! Everything works ok on my local now. Made some changes there. And there's a question in the comment to some code lines in the commit. So, if everything is ok, guess I need to make a pull request since I've read already contributing documentation?

@hpinkos
Copy link
Contributor

hpinkos commented Jul 31, 2017

@avalan4e57 Go ahead and open the pull request. It'll be easier for us to review it that way instead of looking at your individual commits

avalan4e57 added a commit to avalan4e57/cesium that referenced this issue Jul 31, 2017
Set 'updateTimer = undefined' after 'clearTimeout(updateTimer)' call
@avalan4e57
Copy link

avalan4e57 commented Aug 1, 2017

Made pull request #5699 yesterday. Maybe I should mention about it here.

hpinkos pushed a commit that referenced this issue Aug 1, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
good first issue An opportunity for first time contributors type - bug
Projects
None yet
Development

No branches or pull requests

3 participants