fix: prevent silent failure when DOM storage quota exceeded (#20899) (#…

…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.

patches/chromium/dom_storage_limits.patch

@@ -1,80 +1,46 @@
 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
 From: Jacob Quant <jacobq@gmail.com>
-Date: Tue, 6 Nov 2018 15:26:00 -0600
+Date: Thu, 31 Oct 2019 14:00:00 -0500
 Subject: dom_storage_limits.patch
 
-This patch circumvents the restriction on DOM storage objects,
-namely `localStorage` and `sessionStorage`, which chromium otherwise
-limits to approximately 10MiB.
+This patch increases the DOM storage (e.g. `localStorage`
+and `sessionStorage`) size quota from 10MiB to 100MiB.
+Previous versions of this patch attempted to circumvent
+the restriction altogether.
+However, this can lead to other problems, such as crashing
+the Dev Tools when attempting to read or write values that exceed
+`IPC::Channel::kMaximumMessageSize` (128MiB).
 
-That restriction originates from a recommendation
-[in the Web Storage API specification](https://html.spec.whatwg.org/multipage/webstorage.html#disk-space-2)
-that is motivated by the concern that hostile code could abuse this
-feature to exhaust available storage capacity.
-However, in the case of  Electron, where the application developers
-have control over all of the code being executed,
-this safety precaution becomes a hindrance that does not add much value.
-For example, if a malicious developer wanted to consume disk space
-on a victim's machine they could do so via Node's native file system API.
-
-By disabling this restriction or increasing the quota,
-electron application developers can use `localStorage`
-as their application's "back end", without being having
-to limit the amount of data stored to 10MiB.
-
-There may still be some benefit to keeping this restriction for applications that load remote content.
-Although all remote data should be from a trusted source and transferred using
-a secure channel, it is nevertheless advisable to include additional layers of protection
-to mitigate risks associated with potential compromise of those other technologies.
-With that in mind, an acceptable alternative to disabling the limit at compile-time
-(as this patch currently does) would be to instead allow it to be disabled at run-time
-for a given `BrowserWindow` via a `webPreferences` option,
-similar to [`nodeIntegration`](https://electronjs.org/docs/tutorial/security#2-disable-nodejs-integration-for-remote-content).
+Increasing the quota rather than bypassing it reduces the
+amount of chromium code that needs to be changed for Electron
+as well as keeps these storage areas limited to a bounded
+size meanwhile giving application developers more space to work with.
 
 diff --git a/content/browser/dom_storage/dom_storage_types.h b/content/browser/dom_storage/dom_storage_types.h
-index 6c0b831ebaaa2c1749bbc7436ce1025656588310..b67767751cadc6072c133297c7a6cdcc6bfd0c98 100644
+index 6c0b831ebaaa2c1749bbc7436ce1025656588310..96d1c73adb09d33cf591ca569f46de9b21f9a4df 100644
 --- a/content/browser/dom_storage/dom_storage_types.h
 +++ b/content/browser/dom_storage/dom_storage_types.h
-@@ -21,6 +21,7 @@ typedef std::map<base::string16, base::NullableString16> DOMStorageValuesMap;
+@@ -21,7 +21,8 @@ typedef std::map<base::string16, base::NullableString16> DOMStorageValuesMap;
 
  // The quota for each storage area.
  // This value is enforced in renderer processes and the browser process.
-+// However, Electron's dom_storage_limits.patch removes the code that checks this limit.
- const size_t kPerStorageAreaQuota = 10 * 1024 * 1024;
+-const size_t kPerStorageAreaQuota = 10 * 1024 * 1024;
++// Electron's dom_storage_limits.patch increased this value from 10MiB to 100MiB
++const size_t kPerStorageAreaQuota = 100 * 1024 * 1024;
 
  // In the browser process we allow some overage to
-diff --git a/third_party/blink/renderer/modules/storage/cached_storage_area.cc b/third_party/blink/renderer/modules/storage/cached_storage_area.cc
-index d91fdc2a7d52307126bc04d44167edadb8c743a8..630acfca527aaec44742d45e47ce29d7754e3385 100644
---- a/third_party/blink/renderer/modules/storage/cached_storage_area.cc
-+++ b/third_party/blink/renderer/modules/storage/cached_storage_area.cc
-@@ -107,11 +107,13 @@ bool CachedStorageArea::SetItem(const String& key,
-                                 Source* source) {
-   DCHECK(areas_->Contains(source));
-
-+#if 0
-   // A quick check to reject obviously overbudget items to avoid priming the
-   // cache.
-   if ((key.length() + value.length()) * 2 >
-       mojom::blink::StorageArea::kPerStorageAreaQuota)
-     return false;
-+#endif
-
-   EnsureLoaded();
-   String old_value;
-diff --git a/third_party/blink/renderer/modules/storage/storage_area_map.cc b/third_party/blink/renderer/modules/storage/storage_area_map.cc
-index 0da8a1e891edad60355792c40b7d15e90c1086e8..df71418d598d5bdf41e9a8a4340999d9d277aeef 100644
---- a/third_party/blink/renderer/modules/storage/storage_area_map.cc
-+++ b/third_party/blink/renderer/modules/storage/storage_area_map.cc
-@@ -113,10 +113,12 @@ bool StorageAreaMap::SetItemInternal(const String& key,
-   size_t new_quota_used = quota_used_ - old_item_size + new_item_size;
-   size_t new_memory_used = memory_used_ - old_item_memory + new_item_memory;
-
-+#if 0
-   // Only check quota if the size is increasing, this allows
-   // shrinking changes to pre-existing files that are over budget.
-   if (check_quota && new_item_size > old_item_size && new_quota_used > quota_)
-     return false;
-+#endif
+ // accomodate concurrent writes from different renderers
+diff --git a/third_party/blink/public/mojom/dom_storage/storage_area.mojom b/third_party/blink/public/mojom/dom_storage/storage_area.mojom
+index 1f1b2c6fa109aa52c4e7c7f5fcaac91beb538449..d8f15eed9be83340ffd1f2400df08558e9554710 100644
+--- a/third_party/blink/public/mojom/dom_storage/storage_area.mojom
++++ b/third_party/blink/public/mojom/dom_storage/storage_area.mojom
+@@ -40,7 +40,8 @@ interface StorageAreaGetAllCallback {
+ interface StorageArea {
+   // The quota for each storage area.
+   // This value is enforced in renderer processes and the browser process.
+-  const uint32 kPerStorageAreaQuota = 10485760; // 10 MiB
++  // Electron's dom_storage_limits.patch increased this value from 10MiB to 100MiB
++  const uint32 kPerStorageAreaQuota = 104857600; // 100 MiB
 
-   keys_values_.Set(key, value);
-   ResetKeyIterator();
+   // In the browser process we allow some overage to
+   // accommodate concurrent writes from different renderers

spec/chromium-spec.js

@@ -376,16 +376,42 @@ describe('chromium feature', () => {
   })
 
   describe('storage', () => {
-    describe('DOM storage quota override', () => {
+    describe('DOM storage quota increase', () => {
       ['localStorage', 'sessionStorage'].forEach((storageName) => {
-        it(`allows saving at least 50MiB in ${storageName}`, () => {
-          const storage = window[storageName]
-          const testKeyName = '_electronDOMStorageQuotaOverrideTest'
-          // 25 * 2^20 UTF-16 characters will require 50MiB
-          const arraySize = 25 * Math.pow(2, 20)
-          storage[testKeyName] = new Array(arraySize).fill('X').join('')
-          expect(storage[testKeyName]).to.have.lengthOf(arraySize)
-          delete storage[testKeyName]
+        const storage = window[storageName]
+        it(`allows saving at least 40MiB in ${storageName}`, (done) => {
+          // Although JavaScript strings use UTF-16, the underlying
+          // storage provider may encode strings differently, muddling the
+          // translation between character and byte counts. However,
+          // a string of 40 * 2^20 characters will require at least 40MiB
+          // and presumably no more than 80MiB, a size guaranteed to
+          // to exceed the original 10MiB quota yet stay within the
+          // new 100MiB quota.
+          // Note that both the key name and value affect the total size.
+          const testKeyName = '_electronDOMStorageQuotaIncreasedTest'
+          const length = 40 * Math.pow(2, 20) - testKeyName.length
+          storage.setItem(testKeyName, 'X'.repeat(length))
+          // Wait at least one turn of the event loop to help avoid false positives
+          // Although not entirely necessary, the previous version of this test case
+          // failed to detect a real problem (perhaps related to DOM storage data caching)
+          // wherein calling `getItem` immediately after `setItem` would appear to work
+          // but then later (e.g. next tick) it would not.
+          setTimeout(() => {
+            expect(storage.getItem(testKeyName)).to.have.lengthOf(length)
+            storage.removeItem(testKeyName)
+            done()
+          }, 1)
+        })
+        it(`throws when attempting to use more than 128MiB in ${storageName}`, () => {
+          expect(() => {
+            const testKeyName = '_electronDOMStorageQuotaStillEnforcedTest'
+            const length = 128 * Math.pow(2, 20) - testKeyName.length
+            try {
+              storage.setItem(testKeyName, 'X'.repeat(length))
+            } finally {
+              storage.removeItem(testKeyName)
+            }
+          }).to.throw()
         })
       })
     })