browser: Create separate request context for geolocation service.#8923
Merged
browser: Create separate request context for geolocation service.#8923
Conversation
Contributor
|
👍 |
1e5515f to
4f7721e
Compare
Contributor
|
@deepak1556 I saw a crash on the CI machine, seems to be caused by electron-archive/brightray#278: |
* Geolocation service cannot hold reference to browser context,
since it is destroyed at the end of everything and this will
confuse the shutdown path of browser context.
* Geolocation service run on its own thread.
cc9de3c to
1f261ca
Compare
64a01bb to
72adbf7
Compare
Member
Author
|
@zcbenz have fixed the crash, thanks! Depends on electron-archive/brightray#283 |
zcbenz
reviewed
Mar 20, 2017
| void WebContents::WebContentsDestroyed() { | ||
| // This event is only for internal use, which is emitted when WebContents is | ||
| // being destroyed. | ||
| Emit("will-destroy"); |
Contributor
There was a problem hiding this comment.
This changes the meaning of will-destroy, for webview it is emitted before destroying WebContents, however for normal window now it is emitted after WebContents gets destroyed. While this works, it makes the closing logic more difficult to understand.
How about solving the crash with a fix like electron-archive/brightray#284?
Member
Author
There was a problem hiding this comment.
however for normal window now it is emitted after WebContents gets destroyed.
I just tested for normal window close and saw the event emitted before WebContentsDestroyed is called. The change here is that either scenario 1 or 3 of webContents destruction it will always invoke will-destroy before anything else and follow that InspectebWebContentsImpl gets to do the cleanup first. Is there something else that I missed ?
Contributor
There was a problem hiding this comment.
Yeah you are right, I missed how WebContents is destroyed when window is closed.
zcbenz
reviewed
Mar 21, 2017
| // This event is only for internal use, which is emitted when WebContents is | ||
| // being destroyed. | ||
| Emit("will-destroy"); | ||
| CommonWebContentsDelegate::DestroyWebContents(); |
Contributor
There was a problem hiding this comment.
CommonWebContentsDelegate::DestroyWebContents should be made a protected method and renamed to something else like ResetWebContents, having a non-virtual method with the same name is not good practice.
Contributor
|
👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a crash which happens when navigating from a webpage that uses geolocation service and then quitting the app. Test this after applying electron-archive/brightray#278
To reproduce:
youtube.comgoogle.comThe reason for the crash is that geolocation service is managed by a global leaky pointer, since we were holding a reference to browser context, it will therefore be destroyed at the end of everything and messes with the cleanup process.
This also Fixes #8754 , geolocation service has its own dedicated thread, the response from
AtomAccessTokenStorewere being sent back to IO thread instead, hence the UI freezed.