Bug 135330 - Allow window.localStorage to be modified after calling window.close()
Summary: Allow window.localStorage to be modified after calling window.close()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-27 16:22 PDT by Daniel Bates
Modified: 2015-04-23 17:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch and updated tests (24.83 KB, patch)
2014-07-28 12:22 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (25.41 KB, patch)
2014-07-28 16:14 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (25.75 KB, patch)
2014-07-28 16:52 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (26.12 KB, patch)
2014-07-28 16:58 PDT, Daniel Bates
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (554.02 KB, application/zip)
2014-07-28 17:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (495.24 KB, application/zip)
2014-07-28 19:10 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-07-27 16:22:07 PDT
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).
Comment 1 Radar WebKit Bug Importer 2014-07-28 10:34:24 PDT
<rdar://problem/17829985>
Comment 2 Daniel Bates 2014-07-28 12:22:41 PDT
Created attachment 235606 [details]
Patch and updated tests
Comment 3 Sam Weinig 2014-07-28 14:19:38 PDT
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.
Comment 4 Daniel Bates 2014-07-28 16:14:41 PDT
Created attachment 235632 [details]
Patch and layout tests
Comment 5 Daniel Bates 2014-07-28 16:52:48 PDT
Created attachment 235640 [details]
Patch and layout tests
Comment 6 Alexey Proskuryakov 2014-07-28 16:57:04 PDT
Looks like this patch breaks the build on multiple platforms, including Mac.
Comment 7 Daniel Bates 2014-07-28 16:58:27 PDT
Created attachment 235644 [details]
Patch and layout tests
Comment 8 Alexey Proskuryakov 2014-07-28 17:12:12 PDT
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.
Comment 9 Sam Weinig 2014-07-28 17:27:30 PDT
(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.
Comment 10 Build Bot 2014-07-28 17:52:00 PDT
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
Comment 11 Build Bot 2014-07-28 17:52:03 PDT
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
Comment 12 Build Bot 2014-07-28 19:10:46 PDT
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
Comment 13 Build Bot 2014-07-28 19:10:51 PDT
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
Comment 14 Alexey Proskuryakov 2014-07-28 20:56:15 PDT
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).
Comment 15 Daniel Bates 2015-04-23 17:25:06 PDT
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>.