fix: prevent silent failure when DOM storage quota exceeded #20899
Conversation
This file contains 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
jacobq
changed the title
jacobq
changed the title
jacobq
force-pushed
the
jrq/dom_storage_limit
branch
2 times, most recently
from
af4c5dc
to
6f008e2
Compare
|
It seems that the patch is not being applied at the right time or something because I can see the following in static constexpr uint32_t kPerStorageAreaQuota = 10485760U;
static constexpr uint32_t kPerStorageAreaOverQuotaAllowance = 102400U;which I suspect is derived from the unpatched Update: I modified the patch so that it also updates this file now (not sure if there is a better way), and that seems to have done the trick. |
jacobq
force-pushed
the
jrq/dom_storage_limit
branch
2 times, most recently
from
64b7fec
to
5883a33
Compare
jacobq
changed the title
|
FYI The failing mac test appears to be a fluke (failure was caused by time out and the corresponding code should be completely unrelated to this PR). |
|
Please let me know if there's anything that I can do to help land this fix. |
zcbenz
approved these changes
Nov 13, 2019
|
This PR requires approval from @electron/wg-upgrades. |
jacobq
commented
Nov 13, 2019
jacobq
force-pushed
the
jrq/dom_storage_limit
branch
from
5883a33
to
6943fc9
Compare
nornagon
requested changes
Nov 13, 2019
jacobq
force-pushed
the
jrq/dom_storage_limit
branch
from
6943fc9
to
f43bcae
Compare
nornagon
approved these changes
Nov 14, 2019
nornagon
reviewed
Nov 14, 2019
The previous version of this patch did not include
changes required to circumvent the quota enforcement
performed by StorageAreaImpl. Consequently when
the quota was exceeded, things still "appeared to
work" at first but then would later fail silently.
That is, the cache would be updated but the backing
store would not.
This could be fixed by disabling the code below
(from `content/browser/dom_storage/storage_area_impl.cc`)
```
// Only check quota if the size is increasing, this allows
// shrinking changes to pre-existing maps that are over budget.
if (new_item_size > old_item_size && new_storage_used > max_size_) {
if (map_state_ == MapState::LOADED_KEYS_ONLY) {
receivers_.ReportBadMessage(
"The quota in browser cannot exceed when there is only one "
"renderer.");
} else {
std::move(callback).Run(false);
}
return;
}
```
However, since this seems to have some unintended side-effects
(see updated notes in dom_storage_limits.patch) it seems
more prudent to simply increase the quota to a larger
yet still reasonable size rather than attempt to circumvent
the storage quota altogether.
jacobq
force-pushed
the
jrq/dom_storage_limit
branch
from
f43bcae
to
e2d61d2
Compare
nornagon
approved these changes
Nov 14, 2019
|
Could you give the CI a kick? (lint failed due to "Error: socket hang up") |
|
Looks like win-x64 could use a restart as well... the error looks unrelated: |
|
@deepak1556 I'm sure you're busy, but would you mind taking a quick peek at this and letting me know if you notice anything major that needs to be addressed? |
deepak1556
approved these changes
Nov 22, 2019
|
🙏 Thanks, Robo. |
|
Release Notes Persisted
|
jacobq
added a commit
to jacobq/electron
that referenced
this pull request
Dec 4, 2019
…#20899) * test: update DOM storage quota limits test * fix: update dom_storage_limits.patch (fixes electron#13465) The previous version of this patch did not include changes required to circumvent the quota enforcement performed by StorageAreaImpl. Consequently when the quota was exceeded, things still "appeared to work" at first but then would later fail silently. That is, the cache would be updated but the backing store would not. This could be fixed by disabling the code below (from `content/browser/dom_storage/storage_area_impl.cc`) ``` // Only check quota if the size is increasing, this allows // shrinking changes to pre-existing maps that are over budget. if (new_item_size > old_item_size && new_storage_used > max_size_) { if (map_state_ == MapState::LOADED_KEYS_ONLY) { receivers_.ReportBadMessage( "The quota in browser cannot exceed when there is only one " "renderer."); } else { std::move(callback).Run(false); } return; } ``` However, since this seems to have some unintended side-effects (see updated notes in dom_storage_limits.patch) it seems more prudent to simply increase the quota to a larger yet still reasonable size rather than attempt to circumvent the storage quota altogether.
jkleinsc
pushed a commit
that referenced
this pull request
Dec 4, 2019
…21380) * test: update DOM storage quota limits test * fix: update dom_storage_limits.patch (fixes #13465) The previous version of this patch did not include changes required to circumvent the quota enforcement performed by StorageAreaImpl. Consequently when the quota was exceeded, things still "appeared to work" at first but then would later fail silently. That is, the cache would be updated but the backing store would not. This could be fixed by disabling the code below (from `content/browser/dom_storage/storage_area_impl.cc`) ``` // Only check quota if the size is increasing, this allows // shrinking changes to pre-existing maps that are over budget. if (new_item_size > old_item_size && new_storage_used > max_size_) { if (map_state_ == MapState::LOADED_KEYS_ONLY) { receivers_.ReportBadMessage( "The quota in browser cannot exceed when there is only one " "renderer."); } else { std::move(callback).Run(false); } return; } ``` However, since this seems to have some unintended side-effects (see updated notes in dom_storage_limits.patch) it seems more prudent to simply increase the quota to a larger yet still reasonable size rather than attempt to circumvent the storage quota altogether.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of Change
This PR modifies
dom_storage_limits.patchso that instead of (incompletely / incorrectly) circumventing the enforcement of the DOM storage quota, it is simply increased (from 10MiB to 100MiB). The primary motivation for this change is that the previous patch (see PR #15596) does not actually work properly (example).As described in the updated patch notes, bypassing the maximum size enforcement in
StorageAreaImpl::Putpartially resolves the problem but can lead to undesirable side-effects (e.g. instability / poor performance).Cc:
@deepak1556
@alexeykuzmin
@nornagon
Checklist
npm testpassesRelease Notes
Notes: DOM storage quota enforcement re-enabled, but limit changed from 10MiB to 100MiB