1530944 - Legacy LocalStorage impl can desynchronize when all pages for an origin are in the bfcache and retain a LocalStorage reference

Status: RESOLVED WORKSFORME

(Core :: Storage: localStorage & sessionStorage, defect, P2)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

We had a report come in that localStorage values are not being synchronized between tabs. That is, you can get to a state where localStorage.getItem('foo') returns different items for two tabs on the same origin. In my reading of the spec, this seems like a bug. But I note that IE/Edge don't implement the StorageMutex, and thus treat localStorage as isolated.

STR from the reporting email:

• Open two tabs at the same site, e.g. https://example.com
• Open storage inspector in both tabs
• Set a value (in the inspector or with localStorage.setItem) in one of the tabs.
• Navigate tab A to a different site
• In tab B, remove the value or clear localStorage.
• Navigate tab A back again.
  Expected:
  localStorage.getItem('foo') returns the same value in both tabs.

Actual:
Clearing or overriding the key in Tab B, does not change the value in Tab A.

Verification Notes

I've tested this and verified that the localStorage.getItem() ends up returning different items in different tabs. Its even different in storage inspector.

I tried also to see what happens in if I flip the dom.storage.next_gen pref (due to the discussion in 1453699) but I observed the same behavior (on linux). I tested in release (65) and nightly (67)

NB, the original reporter was concerned of the security issue around this, due to session tokens stored in localStorage not being properly cleared. While that may be an issue, this doesn't sound like a situation that is directly exploitable, and moreoever would benefit from being public (ie so developer are aware of the bug and can avoid it). So im leaving this bug public.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

FYI i just realised I didnt test properly with NGLS - i didnt restart browser after pref flip. So maybe NGLS fixes this.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Just re-tested, and this IS fixed with the NGLS pref enabled. So I guess this is fixed from 66 onwards (where the pref is enabled)

Comment 3

[Jan Varga [:janv]]

LSNG is not enabled by default, but if everything goes well, it will be enabled in 67.
See bug 1517090.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Interesting. So this is a new variation on a problem we have in a few other edge-cases.

Specifically this is the intersection of:

• legacy LocalStorage only applies changes at an nsGlobalWindowInner level and doesn't process the changes if it's frozen (which happens when it's in the BFCache)
• pages retain a reference to their mLocalStorage while they're in the BFCache.

I think there is a mitigation we can land:

• Ensure that if we take the if (!IsCurrentInnerWindow() || IsFrozen()) early-return branch case in nsGlobalWindowInner::ObserveStorageNotification[1] that we effectively invoke CloneStorageEvent for side-effect but don't dispatch it. This will cause the LocalStorageCache to be updated. Since we're not actually going to dispatch the event, we could put logic in that branch that basically looks like:

      // A null localStorage event is being propagated by IPC and needs to be applied
      // if we retain a reference to a LocalStorage implementation, otherwise the
      // cache will become out-of-date.
      //
      // We don't want to apply if:
      // - LocalStorage NextGen is enabled.  It does not propagate state this way.
      // - The window hasn't accessed LocalStorage.  If we don't have the data, there's
      //   no need to keep it in-sync.
      // - This window is using the PartitionedLocalStorage implementation because of
      //   tracking protection.  It must not see changes from non-partitioned
      //   implementations.
      if (!aEvent->GetStorageArea() && !NextGenLocalStorage() && mLocalStorage &&
          mLocalStorage->Type() == Storage::eLocalStorage) {
        RefPtr<LocalStorage> localStorage =
            static_cast<LocalStorage*>(mLocalStorage);

        // We must apply the current change to the 'local' localStorage.
        localStorage->ApplyEvent(aEvent);
      }

Pretty much all other mitigations are bad:

• Prevent the page from going into bfcache. This is bad because many pages use LocalStorage and this could largely break bfcache.
• Drop mLocalStorage upon entry into the bfcache. This is bad because we might block the main thread after coming out of the BFCache if the page goes to access the data against.
• Queue the events. There used to be logic that attempted to do this prior to the e10s enhancements, but it never worked but managed to be a serious memory leak risk. We don't want to potentially leak memory either.
• Process the events: We're not allowed to fire JS on a frozen window.

1: https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/dom/base/nsGlobalWindowInner.cpp#5249

Comment 5

[Andrew Overholt [:overholt]]

We're going to decide tomorrow whether or not lsng rides in 67 so let's make a call on the potential workaround after the lsng go/no go decision is made.

Comment 6

[Jens Stutte [:jstutte]]

LSNG finally shipped some time ago, though not exactly in 67...