-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
[Bug]: Geolocation crashes 13.0.1 #29343
Comments
Crash occurs on x86_64 and arm64. |
Here is a gist with a repro: https://gist.github.com/andrewpaprotsky/c46eb4368e236d6ed0a70d3fbb2054ac |
Did some bisecting with Andrew's Electron fiddle:
|
[ deleted incorrect post as explained later ] |
Can reproduce in 13.1.0 too |
Okay, this is kinda weird. This is crashing on 13.1.0, 14.0.0-beta.2 and the latest 15 nightly. The reason it's weird is that when I posted about that commit fixing it, I downloaded the artifacts from Circle CI: And used that as a Local Electron Build in Electron Fiddle. It doesn't crash. However, to get that artifact to run I had to
Hmmm. |
Okay, so with the 13.1.0 artifacts from Circle CI: If I If I use the official 13.1.0 build, it crashes. If I download 13.1.0 from the GitHub releases page (
I'm not really clear on how the assets on the releases page are generated. |
@omarkilani Artifacts on CI are built using our Testing config, artifacts we release are built using the Release config. This can in rare cases result in differences in behavior. (Just explaining what you're seeing) |
@MarshallOfSound that makes sense. Thanks! I was trying to find the Circle CI release job or something to test those artifacts. So... if this doesn't crash the release build of Chrome 91 (which now matches Electron) and doesn't crash the debug build of Electron I guess it makes it fun? :) There are four commits between 13.0.0.beta.13 and 13.0.0.beta.14. Two of them might be the culprit but I'm not sure how to check if a cherry picked v8 patch is still part of the upstream v8 or in the release build Chrome. |
Okay so with commit 7044cb6
So I think that leaves something in 66a2218 . |
You want to find the https://github.com/electron/electron/runs/2337296361 I suspect it's something in the Chrome roll we didn't account for / went past without a build failure. Just to clarify, is this definitively macOS only? |
@MarshallOfSound I just tested on Win 10 x64 and Ubuntu 20.04 64-bit and it doesn't crash there. So yup, macOS only. |
I can confirm, we have tested in Windows, Linux, and Mac. Of those only Mac segfaults on Geolocation. |
Yes only the Mac crashes using x64 11.4 |
Issue still present in 13.1.1 |
@MarshallOfSound I’d love to take a look at this further but I can’t get Electron to build on my M1 MBP. I followed the steps in the building docs but I might have missed something. Is there somewhere one can go to ask possibly… simple… building questions? |
Having this issue too when trying load google.com on MacOS x64 inside electron with everything after 13.0.0.beta.13 |
This appears resolved for me in 13.1.2 |
@TheCleric we still see the crash in 13.1.2. And @andrewpaprotsky 's fiddle still crashes with Electron Fiddle + 13.1.2. There don't appear to have been any new releases in the last 7 days? |
@omarkilani you're right! Just tried again in my original test and it did fail. The difference between the two (for anyone actively working this) was that in my working test I had overridden the user agent to use a Firefox user agent. Not sure if that helps narrow down the issue or not. |
Uh, this is funny I totally figured out what was causing this crash two weeks ago and have since closed all my tabs and lost all my research 😆 I'll figure this out again 😄 |
Who among us has not done this. 😆 That and "I fixed the code in a dream, but I'll be damned if I can remember how..." |
Strange how the dream fixes always seem so easy and still not possible to remember :-) Don't know if this can help with the debugging but it may help as a workaround to others in the same boat as us.
|
@MarshallOfSound, worth noting that this doesn't seem to crash anymore on EDIT: Hacked up my Fiddle so I could test the newer nightlies, looks like it still crashed as of |
This solution worked. App is not crashing anymore when navigating to any URL which asks for GeoLocation permission and crashed the entire app without this fix. |
FWIW, still crashing in: 13.1.4 |
So based on @omarkilani 's research above, this looks like it broke between 13.0.0-beta13 and 13.0.0-beta14. Looking at the diff, only thing I can (naively) notice that might be an issue is the chromium version bump: v13.0.0-beta.13...v13.0.0-beta.14 Which went from So I started digging through chromium compares between those two versions, but that revealed to me that I am way out of my depth there. 😆 I'll leave this to those that are wiser than I. |
I also stumbled across this bug in Chromium's issue tracker. Not sure if it's related as it's talking about headless browser usage, but it seems pretty close: The issue there is that headless chromium couldn't access |
@TheCleric I tried to narrow down which commit the issue was introduced in here: #29343 (comment) I wasn't successful in building Electron on my main M1 machine, so I switched to other tasks. I've got it downloading on a x86_64 machine now then I'll try to take a deeper look. |
Hmmmmm.... well, the crash is in:
And this was one of the changes in 66a2218:
Maybe some default changed somewhere? 🤔 Edit: this patch was removed in f72597b#diff-849f1c17197312f1191586b602a4115a2efe3fba15c9f7ecd4170cf2c7d5bea2 Hmmm. |
That is interesting @omarkilani Especially since your bug was filed the same day as this article I found: https://www.theregister.com/2021/05/26/chrome_91_release/
So here's my likely incorrect wild-ass guess/theory: Google is trying to use WASM for GeoLocation if it detects Chrome 91+
As well, I can only get this crash to happen on Google sites (such as google.com) though I'd suspect some other sites that use Google Analytics might crash as well. Interestingly to you @omarkilani , I ran rememberthemilk.com through Nativefier (a web site to Electron app generator that I help maintain) on our experimental electron_13 branch, and it did not crash on my Mac here, so I don't know if your customized app has some geolocation features that the site does not. 🤔 |
@TheCleric In our web app, we only call out to Geolocation if the user uses a Geolocation feature. I.e. one of our location-based search operators ( The only Geolocation-related customisation in our Electron app is setting a Google API key. |
If I run the latest 13_x_y testing arm64 artifacts: Using it as a Local Electron with:
(C.f. #29343 (comment)) And test SIMD support with
So yeah, once Or the mention of WASM SIMD triggers something in @MarshallOfSound 's memory and he knows what to do. :) |
Okay, so I managed to get Electron built (Testing + Release took 8 hours) and... discovered that the crash that occurs on 13.x is different to the one on 15.x. So... the one on 13.x:
And the one on 15.x:
Unfortunately I built the But I guess that's something to look into on 15.x. Or I might be doing something wrong with entitlements? I did code sign all the .app's with my own cert but that didn't change anything. |
@TheCleric FWIW, I now think that headless Chrome bug might be doing something, but I'm not sure what. Slightly different backtrace on 15.x but the cause is the same:
That change landed in: https://chromiumdash.appspot.com/commit/65d98b3a63fc9c2fb2ce30898d103f7a343daefa So, 90.0.4430.19. 13.0.0.beta13 shipped with: Line 17 in 1f95fdd
So, 90.0.4415.0. Then beta14 bumped it to include that fix. What's not clear (yet) is why it crashes only on the Release (maybe something different gets set on Or maybe something else changed in 15.x and the way Electron hooks into Chromium is different than on 13.x/14.x. 🤷 FWIW, these are the two crashes shown in the Mac crash reporter for 13.1.4 and 14.0.0-beta.9 with the official Electron builds running against https://www.google.com/, i.e.:
Edit: hmmmm... but the "official" Electron 15 nightly crashes with the WASM SIMD backtrace, too:
Maybe the self-compiled binaries are different somehow. |
Okay so mystery solved, I guess. Without dSYMs:
With dSYMs:
So yeah, that is the reason, @TheCleric 👍. The Mac crash reporter lied. :) No idea why it doesn't crash on non-Release builds 🤷 . Anyway, the cause of the crash: https://chromium.googlesource.com/chromium/src/+/36d366175fee2d4f0fd0a8ccf53338984da9b531%5E%21/ The fix for headless Chrome: https://chromium.googlesource.com/chromium/src/+/39cabc596fccd3e79b71cd8ddd0f3348cc2975d9%5E%21/ I'll try to work up some patches. There's a slight difference in that on Chromium 91, the class is called |
Preflight Checklist
Electron Version
13.0.1
What operating system are you using?
macOS
Operating System Version
11.4
What arch are you using?
arm64 (including Apple Silicon)
Last Known Working Electron version
12.0.7
Expected Behavior
Electron shouldn't crash.
Actual Behavior
Electron crashes.
Testcase Gist URL
https://gist.github.com/omarkilani/f0c264e278b04575a381d1eadc930514
Additional Information
Electron 13.x crashes on any geolocation use. Backtrace attached.
The text was updated successfully, but these errors were encountered: