Bug 211503 - Regression(r254859) DOM storage event gets fired at the frame that caused the storage modification
Summary: Regression(r254859) DOM storage event gets fired at the frame that caused the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Safari 13
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 206433
  Show dependency treegraph
 
Reported: 2020-05-06 03:47 PDT by pirimoglu
Modified: 2020-05-22 11:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.73 KB, patch)
2020-05-21 16:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pirimoglu 2020-05-06 03:47:43 PDT
With Safari 13.1 update, DOM storage event is being raised at the same window that has caused the storage event to happen. This is a behavior difference from earlier versions of Safari, and is not complaint with the HTML Living Standard specification on this. The standard spec at https://html.spec.whatwg.org/multipage/webstorage.html#localStorageEvent  says this for localStorage events:

"When the setItem(), removeItem(), and clear() methods are called on a Storage object x that is associated with a local storage area, if the methods did not throw an exception or "do nothing" as defined above, then for every Document object whose relevant global object's localStorage attribute's Storage object is associated with the same storage area, other than x, send a storage notification."

And says this for sessionStorage events:

"When the setItem(), removeItem(), and clear() methods are called on a Storage object x that is associated with a session storage area, if the methods did not throw an exception or "do nothing" as defined above, then for every Document object whose relevant global object's sessionStorage attribute's Storage object is associated with the same storage area, other than x, send a storage notification."

For both localStorage and sessionStorage, the a "storage notification" should be sent for every document whose corresponding storage object is the same one as the current document, which they call "x", but not on "x". So the document who made the setItem(), removeItem() or clear() methods, should not receive the storage notification (event). Only others that are in the same session who share the same storage object should receive the notification.

Before Safari 13.1 it was behaving this way. With Safari 13.1 it is not behaving this way, the document that called the methods that causes the notification (event) is also receiving the event.

To give example in js code:

Assume document A has this code: 
   window.addEventListener("storage", storageEventHandler);

   function storageEventHandler(e) {
      console.log("A received " + e.key);
   }

function AddToLocalStorage(key, value)
{
   localStorage.setItem(key, value);
}

And document B has this code, which is loaded from same origin/site as document A, so they share session and so localStorage:

   window.addEventListener("storage", storageEventHandler);

   function storageEventHandler(e) {
      console.log("B received " + e.key);
   }

Assume both document A and document B are loaded and the event listeners are set.
Assume on document A, AddToLocalStorage("new key" , "value1") is called, by a button click or some other way.

In this case, because document A is calling setItem(), document A's storegaEventHandler() should not be invoked. But document B's storageEventHandler() should be invoked. 

With Safari 13.1 document A's storageEventHandler is being invoked too.
This is against the specification, and breaks existing functionality that relies on this. Only IE 10 behaves this way, but it is known to not be complaint with specifications.
Comment 1 Radar WebKit Bug Importer 2020-05-07 10:53:32 PDT
<rdar://problem/62983284>
Comment 2 Chris Dumez 2020-05-21 15:06:01 PDT
Will fix shortly. Thanks for the bug report.
Comment 3 Chris Dumez 2020-05-21 16:37:30 PDT
Created attachment 399997 [details]
Patch
Comment 4 Chris Dumez 2020-05-21 16:51:53 PDT
pirimoglu@hotmail.com: I don't think we shipped this bug yet. Were you testing Safari Technology Preview?
Comment 5 pirimoglu 2020-05-22 00:18:06 PDT
No, released product. It starts with Safari 13.1, from end user complaints and what we tested.
Comment 6 pirimoglu 2020-05-22 01:45:10 PDT
I think you are testing your fix with only window.sessionStorage, but not with window.localStorage. 

I went back and tested this with the Safari 13.1, both with sessionStorage and localStorage. In Safari 13.1, sessionStorage is working as expected. But localStorage has this problem, which is what our javascript is using on our site.
Comment 7 Maciej Stachowiak 2020-05-22 01:46:45 PDT
Comment on attachment 399997 [details]
Patch

r=me
Comment 8 EWS 2020-05-22 07:28:40 PDT
commit-queue failed to commit attachment 399997 [details] to WebKit repository.
Comment 9 Chris Dumez 2020-05-22 08:17:15 PDT
Comment on attachment 399997 [details]
Patch

Clearing flags on attachment: 399997

Committed r262058: <https://trac.webkit.org/changeset/262058>
Comment 10 Chris Dumez 2020-05-22 08:17:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 pirimoglu 2020-05-22 09:02:20 PDT
Can you please confirm or verify if the fix with localStorage still work when there are two or more browser tabs open that share the same localStorage, due to being on same url or same origin?

If one of these windows had modified the localStorage, the other windows should receive the storage event, but not the originating window. Our javascript code is relying on this. Wanted to confirm this will still be case and not regress. 

Just in case this may be behaving differently than iframe on the same page case.

Thanks for the fix!
Comment 12 Chris Dumez 2020-05-22 11:54:53 PDT
(In reply to pirimoglu from comment #11)
> Can you please confirm or verify if the fix with localStorage still work
> when there are two or more browser tabs open that share the same
> localStorage, due to being on same url or same origin?
> 
> If one of these windows had modified the localStorage, the other windows
> should receive the storage event, but not the originating window. Our
> javascript code is relying on this. Wanted to confirm this will still be
> case and not regress. 
> 
> Just in case this may be behaving differently than iframe on the same page
> case.
> 
> Thanks for the fix!

Yes, I have just verified that it works as expected, even with multiple tabs. You should be able to validate this too in the next Safari Technology Preview build.
Comment 13 pirimoglu 2020-05-22 11:59:06 PDT
Thanks again!