Closed Bug 1372662 Opened 3 years ago Closed 3 years ago

Don't copy SessionStore when creating a window with noopener

Categories

(Core :: Window Management, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: nika, Assigned: nika, NeedInfo)

References

Details

Attachments

(2 files, 5 obsolete files)

We need this right now in order to implement bug 1370971 according to spec, but we would like to not need to do this (see https://github.com/whatwg/html/issues/2681).

Let's implement this behind a pref so we can decide what we want.
This patch implements the feature in the simplest way I could think of, which is
using the copy of sessionStorage in sessionstore.

Ideally we'd have a better solution for this.

Doing a try run before I ask for review.

MozReview-Commit-ID: 6f3PMyZSAkd
MozReview-Commit-ID: lvLAfuZYFm
Attachment #8878143 - Flags: review?(bzbarsky)
Attachment #8877751 - Attachment is obsolete: true
This was the easiest way I could think of to copy SessionStorage between browsers across processes right now. Baku, I was told you might know things about the future of LocalStorage and SessionStorage, and perhaps you might have an idea of a better/easier way to do this?

Holding off on asking for review on the remaining parts of this bug until I get feedback :).

MozReview-Commit-ID: 2Hfw3SWcLAY
Attachment #8878145 - Flags: feedback?(amarchesini)
Comment on attachment 8878143 [details] [diff] [review]
Part 1: Update the browser_noopener test to also test sessionstorage copying

r=me, I think.  I assume the test at least fails without these changes and passes with.  ;)
Attachment #8878143 - Flags: review?(bzbarsky) → review+
This actually doesn't work very well (I mess up the loading semantics in part 3, and that means that if we open too many windows at once you can race and fail to open some of them after my changes).

I can probably adapt this patch to make it work (I'd have to probably start the SessionStore flush on the child side before sending the request, and then read it on the parent side, shoving propagating some reference to the SessionStorage information through the Js code which triggers the initial load). It would be awesome if there was a better way to do this, or if we had plans to change SessionStorage already which would make my life easier.
Flags: needinfo?(amarchesini)
Attachment #8878145 - Flags: feedback?(amarchesini)
According to the comments in https://github.com/whatwg/html/issues/2681 it looks like this is what the spec is going toward.
Attachment #8901967 - Flags: review?(bugs)
Attachment #8878143 - Attachment is obsolete: true
Attachment #8878145 - Attachment is obsolete: true
Attachment #8878146 - Attachment is obsolete: true
Attachment #8878147 - Attachment is obsolete: true
I'm also hoping that this will cover the request for a test in the linked whatwg/html bug.
Attachment #8901968 - Flags: review?(bugs)
Attachment #8901968 - Flags: feedback?(annevk)
Summary: Copy SessionStorage into windows created with CreateWindowInOtherProcess → Don't copy SessionStore when creating a window with noopener
Comment on attachment 8901967 [details] [diff] [review]
Part 1: Never copy sessionstorage into noopener windows

Looks rather simple :)
Attachment #8901967 - Flags: review?(bugs) → review+
Comment on attachment 8901968 [details] [diff] [review]
Part 2: Add wpt tests for not copying sessionstorage into noopener windows

BroadcastChannel is so nice.
Attachment #8901968 - Flags: review?(bugs) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb9b3c58699
Part 1: Never copy sessionstorage into noopener windows, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2af34cba4d4c
Part 2: Add wpt tests for not copying sessionstorage into noopener windows, r=smaug
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d6cf31353d
Part 3: Include manifest changes for WPT on a CLOSED TREE, a=bustage
Attachment #8901968 - Flags: feedback?(annevk) → feedback+
You need to log in before you can comment on or make changes to this bug.