Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup how disowning works #4318

Closed
wants to merge 1 commit into from
Closed

Cleanup how disowning works #4318

wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 23, 2019

@annevk
Copy link
Member Author

annevk commented Jan 23, 2019

I think this ends up being editorial, but it makes it even more painfully clear how true #3874 by @gterzian is. That currently disowning has no noticeable effect.

Perhaps that's okay though and better than the status quo. I could change my source comment into an issue marker for #313 and at least it'd be a little more transparent?

@annevk
Copy link
Member Author

annevk commented Jan 23, 2019

I did some of this in #4284 so I thought that perhaps landing this separately would be good so not too much is done in that PR.

@annevk annevk requested a review from domenic January 23, 2019 15:20
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble understanding all the consequences here, in this intermediate world. Can you help me fill this in, so I can better review this and #4284?

  • noopener link relations:
    • impacts navigation via "the rules for choosing a browsing context". Basically seems to only affect sessionStorage copying now?
    • No longer sets the disowned boolean
    • Thus, window.opener will return the opener.
    • This seems like a regression.
  • opener setter/getter:
    • The setter sets the "disowned" boolean, which the getter consults. No further impact (i.e. doesn't change auxiliary vs. top-level, or anything else).
  • window.open() with the noopener feature:
    • Goes down "the rules for choosing a browsing context" path, which as far as I can tell only consults noopener for sessionStorage copying.
    • No longer sets the disowned boolean
    • Thus, window.opener will return the opener.
    • This seems like a regression.

<p>The keyword indicates that any newly created <span>top-level browsing context</span> which
results from following the <span>hyperlink</span> is not an <code>auxiliary browsing
context</code>. E.g., its <code data-x="dom-opener">window.opener</code> attribute will be
null.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to link to where the processing model implements this, as an addition.

following the <span>hyperlink</span> will be <span>disowned</span>, which means that its <code
data-x="dom-opener">window.opener</code> attribute will be null.</p>
<p>The keyword indicates that any newly created <span>top-level browsing context</span> which
results from following the <span>hyperlink</span> is not an <code>auxiliary browsing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code -> span

<p>The keyword indicates that any newly created <span>top-level browsing context</span> which
results from following the <span>hyperlink</span> is not an <code>auxiliary browsing
context</code>. E.g., its <code data-x="dom-opener">window.opener</code> attribute will be
null.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example would be nice. A link that would normally create an auxiliary browsing context, plus one with noopener that does not.

<li><p>Return the current <span>browsing context</span>'s <span>opener browsing context</span>'s
<code>WindowProxy</code> object.</p></li>
</ol>

<p>The <code data-x="dom-opener">opener</code> attribute's setter, must run these steps:</p>

<ol>
<li><p>If the given value is null, then <span data-x="disowned">disown</span> the current
<li><p>If the given value is null and the current <span>browsing context</span> is an
<span>auxiliary browsing context</span>, then <span data-x="disowned">disown</span> the current
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a normative change to fall back to step 2 for non-auxiliary browsing contexts, instead of returning. (Well, the previous spec would probably crash, since it would try to call "disown" on a non-auxiliary BC, which per its definition is not allowed. But I think a reasonable reading is that it would return.)

Have you tested what gets done in browsers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm merging this into the other PR, but this is a good question that needs an answer either way.

const beforeDesc = Object.getOwnPropertyDescriptor(self, "opener"),
      openerGet = beforeDesc.get;

alert(openerGet());
self.opener = null;
self.opener = "test";
alert(openerGet());

in an iframe targeted with window.open('theabovedocument.html', 'theiframename') that alerts Window followed by null in Firefox. Chrome and Safari throw on openerGet() however. So yeah, disowning needs to affect all browsing contexts. I'm pretty sure from other tests it only affects link targeting when it's done on a auxiliary browsing context though.

@annevk
Copy link
Member Author

annevk commented Jan 24, 2019

I guess I have to merge this into #4284. noopener should create a non-auxiliary top-level browsing context. I.e., one without an opener browsing context.

Disowning only affects auxiliary browsing contexts. In particular it makes the browser skip those browsing contexts when doing name lookup. It doesn't really severe the opener browsing context relationship as someone could have already obtained a reference.

annevk added a commit that referenced this pull request Jan 24, 2019
@annevk
Copy link
Member Author

annevk commented Jan 24, 2019

Closing in favor of #4318, which now captures everything captured here.

@annevk annevk closed this Jan 24, 2019
@annevk annevk deleted the annevk/disown-cleanup branch January 24, 2019 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants