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

Fix chrome storage access scope #9951

Merged
merged 6 commits into from Jul 24, 2017

Conversation

Projects
None yet
3 participants
@alexstrat
Contributor

alexstrat commented Jul 7, 2017

Fixes #9949

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Jul 7, 2017

Member

I'm not sold on this approach personally, there are a lot of edge cases and things to handle when persisting things to disk. You definitely want to at least be using the async fs methods with some form of back-off and retry logic for when things don't work the first time.

You also need to recursively make the parent directories if they don't exist.

I'd much rather find a way to pass the persistence of this data off to Chromium logic that has already been battle tested

Member

MarshallOfSound commented Jul 7, 2017

I'm not sold on this approach personally, there are a lot of edge cases and things to handle when persisting things to disk. You definitely want to at least be using the async fs methods with some form of back-off and retry logic for when things don't work the first time.

You also need to recursively make the parent directories if they don't exist.

I'd much rather find a way to pass the persistence of this data off to Chromium logic that has already been battle tested

@alexstrat

This comment has been minimized.

Show comment
Hide comment
@alexstrat

alexstrat Jul 7, 2017

Contributor

Definitely ok for using async fs methods and recursively mkdir the parent directories => will do so. That's the way to do it properly!

I'd much rather find a way to pass the persistence of this data off to Chromium logic that has already been battle tested

Do you have somthing in sight? I think every DOM storage methods will be scoped by domain and I'm not aware of other Chromium-based persistence methods already implemented and battle-tested.

Contributor

alexstrat commented Jul 7, 2017

Definitely ok for using async fs methods and recursively mkdir the parent directories => will do so. That's the way to do it properly!

I'd much rather find a way to pass the persistence of this data off to Chromium logic that has already been battle tested

Do you have somthing in sight? I think every DOM storage methods will be scoped by domain and I'm not aware of other Chromium-based persistence methods already implemented and battle-tested.

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Jul 7, 2017

Member

Do you have somthing in sight?

Never really looked into the implementation of storage in Chromium too much, @zcbenz might know if their is a Chromium API we can expose and use here.

Member

MarshallOfSound commented Jul 7, 2017

Do you have somthing in sight?

Never really looked into the implementation of storage in Chromium too much, @zcbenz might know if their is a Chromium API we can expose and use here.

@alexstrat

This comment has been minimized.

Show comment
Hide comment
@alexstrat

alexstrat Jul 11, 2017

Contributor

I've implemented the use of async fs as well as recursive mkdir.

..with some form of back-off and retry logic for when things don't work the first time

I'd be glad to do it, but how and why things would not work for the first time? Other parts of electron use persisted data on disk and it does not look like to me they implement some sort of back-off. Is that because we are on renderer side? If so, wouldn't be wise to centralise these access in the main process and deal via IPCs.


I've looked into Chromium's storage related to chrome.storage and here is my interpretations:

Contributor

alexstrat commented Jul 11, 2017

I've implemented the use of async fs as well as recursive mkdir.

..with some form of back-off and retry logic for when things don't work the first time

I'd be glad to do it, but how and why things would not work for the first time? Other parts of electron use persisted data on disk and it does not look like to me they implement some sort of back-off. Is that because we are on renderer side? If so, wouldn't be wise to centralise these access in the main process and deal via IPCs.


I've looked into Chromium's storage related to chrome.storage and here is my interpretations:

Show outdated Hide outdated lib/renderer/extensions/storage.js Outdated
@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Jul 17, 2017

Contributor

This change would make old apps lose their data in storage, but I guess it is fine since it only affects devtools extensions.

Contributor

zcbenz commented Jul 17, 2017

This change would make old apps lose their data in storage, but I guess it is fine since it only affects devtools extensions.

@zcbenz

zcbenz approved these changes Jul 24, 2017

@zcbenz zcbenz merged commit 25f168c into electron:master Jul 24, 2017

3 of 5 checks passed

electron-win-x64 Build #3604 failed in 26 min
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
electron-mas-x64 Build #4624 succeeded in 10 min
Details
electron-osx-x64 Build #4626 succeeded in 10 min
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment