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: bypass DOM storage quota #15596
Conversation
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
Although the patch looks good, I would like to get a better understanding of the side effects from changing the rate limiter for writes, storage-dev would be the best place to get more thoughts on this approach. |
I just threw that in there "just in case" as, once again, it seems like something that shouldn't be restricted in Electron. If it will help land this sooner I will take that out. |
1912cb2
to
a9938de
Compare
@deepak1556 I removed the change about which you had concerns. Would you mind reviewing again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can you add a description in the patch file for why it is needed, #15612 is a good example. Thanks!
@deepak1556 Sure, description starts here. More discussion may also be found in the related issues. |
Could this also be backported to 3-0-x? |
Note: failed win-ia32 test appears unrelated. Would one of the releasers please restart the failed build(s)? |
Similar to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM , Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
spec/chromium-spec.js
Outdated
@@ -937,6 +937,23 @@ describe('chromium feature', () => { | |||
}) | |||
|
|||
describe('storage', () => { | |||
describe('DOM storage quota override', () => { | |||
['localStorage', 'sessionStorage'].forEach((storageName) => { | |||
it(`can store at least 50MiB in ${storageName}`, (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A done
callback is not needed for synchronous tests, please remove it.
@jacobq |
Co-Authored-By: jacobq <jacobq@gmail.com>
@alexeykuzmin OK, I just added some more commits and think I got all the changes you requested. Once this lands I'll open a separate PR for 3.0.x backport. Edit: Looks like it won't be quite as simple as committing the same patch file because the hash of the files being patched has changed. So I think I'll have to regenerate it against chrome 66, not a biggy, but not a 5 minute ordeal for me either. |
Hello again...would someone please re-run the 2 failed CI jobs? (mas-testing-tests & osx-testing-tests) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobq Thank you for the fix.
I just restarted those CI jobs.
Release Notes Persisted
|
Congrats on merging your first pull request! 🎉🎉🎉 |
I have automatically backported this PR to "4-0-x", please check out #15686 |
Description of Change
This updates the
dom_storage_map
patch to include bypassing the 2 additional locations of quota checking. It also adds a test to ensure that at least 50MiB of data can be stored inlocalStorage
andsessionStorage
(I'm surprised that I didn't find something like that already in place. Perhaps that's how the regression sneaked in.) Since the usual limit is about 10MiB the test will fail if the patch has not been applied or is not working.Fixes #8337.
cc @MarshallOfSound you mentioned you might be willing to help land this. (Thanks in advance!)
cc @zcbenz It looks like you wrote the original patch. (Thank you! That helped me understand how electron's build system applies chromium patches)
cc @asinbow You pointed out the additional checks that needed to be disabled. (Thank you!)
Checklist
npm test
passes (CI is green)Note about documentation:
Since this is fixing a regression (doing what the previous patch used to do), I don't think there's any need to change documentation. However, it might make sense to record this behavior in the documentation somewhere (I couldn't find it). Perhaps in the FAQ where
localStorage
is mentioned.Note about running tests locally:
It seems that I can't run the test suite locally, though I can build a release and manually test in the console. Here's what I get on my (Debian 9.5) machine when I try to build the debug version (needed for running tests, right?) It seems like my system's libc version does not match the required one.
Release Notes
notes: Fixes regression regarding
localStorage
quota not being bypassed.