Bug 135330 – Allow window.localStorage to be modified after calling window.close()

• Component: WebCore Misc.
• Status: NEW

[Daniel Bates]

Similar to other browsers and WebKit1 before the patch for bug #135328, we should consider allowing access and modifications to local storage after window.close() is called regardless of whether local storage was accessed before the call to window.close().

As of the time of writing, such behavior is supported in Firefox for Mac version 31.0, Chrome for Mac version 37.0.2062.44, and IE 11 (11.0.9600.17207).

[Radar WebKit Bug Importer]

<rdar://problem/17829985>

[Daniel Bates]

Created attachment 235606 [details]
Patch and updated tests

[Sam Weinig]

Comment on attachment 235606 [details]
Patch and updated tests

I think we should go with an attempt where we just stop changing the PageGroup during closing.

[Daniel Bates]

Created attachment 235632 [details]
Patch and layout tests

[Daniel Bates]

Created attachment 235640 [details]
Patch and layout tests

[Alexey Proskuryakov]

Looks like this patch breaks the build on multiple platforms, including Mac.

[Daniel Bates]

Created attachment 235644 [details]
Patch and layout tests

[Alexey Proskuryakov]

Comment on attachment 235644 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=235644&action=review

> Source/WebCore/ChangeLog:11
> +        We should allow window.localStorage to be modified after calling window.close() in
> +        both WebKit1 and WebKit2 so as to match the behavior observed in other browsers. This
> +        patch represents a partial revert of <https://trac.webkit.org/changeset/171661>.

I wonder what happens if we write to localStorage, and exceed its quota. There isn't a window any more to drop a confirmation sheet from.

As far as I know, Brady is not on board with this change. We should get the concerns resolved.

[Sam Weinig]

(In reply to comment #8)
> (From update of attachment 235644 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235644&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        We should allow window.localStorage to be modified after calling window.close() in
> > +        both WebKit1 and WebKit2 so as to match the behavior observed in other browsers. This
> > +        patch represents a partial revert of <https://trac.webkit.org/changeset/171661>.
> 
> I wonder what happens if we write to localStorage, and exceed its quota. There isn't a window any more to drop a confirmation sheet from.
> 
> As far as I know, Brady is not on board with this change. We should get the concerns resolved.

Presumably, the same thing that happens if you grab the localStorage object before the window closes.

[Build Bot]

Comment on attachment 235644 [details]
Patch and layout tests

Attachment 235644 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5302771481837568

New failing tests:
fast/dom/Window/dom-access-from-closure-window-with-gc.html

[Build Bot]

Created attachment 235654 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 235644 [details]
Patch and layout tests

Attachment 235644 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6517817729875968

New failing tests:
fast/dom/Window/dom-access-from-closure-window-with-gc.html

[Build Bot]

Created attachment 235656 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Alexey Proskuryakov]

Dan also looked into what happens when a script accesses its own localStorage immediately after calling window.close() vs. what happens when accessing localStorage of a separate window/frame that was previously closed.

If I understood the outcome correctly, this patch results in an inconsistency between these cases, and doesn't bring us in line with other browsers (which handle it consistently).

[Daniel Bates]

Comment on attachment 235644 [details]
Patch and layout tests

Clearing review flag since it is unclear whether we want to support such behavior. As of the time of writing (04/23/2015), there has not been any bugs filed or known websites affected by the lack of such behavior following the patch for bug #135328, <https://trac.webkit.org/changeset/171661>.