Boris Zbarsky [:bzbarsky] Assignee
	Description • 8 years ago

See <https://github.com/whatwg/html/pull/290> and the original bug 1222516 we had for this: the idea is that you can do:

  var w = window.open(foo, "_blank", "noopener")

and foo will not have access back to you via window.opener.

See bug 1222516 comment 3 for the issues that need to be sorted out to implement this.

I've spun the cleanup I mention there out into bug 1267338.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 8 years ago

I filed bug 1306519 on some followup work here.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 8 years ago

Comment on attachment 8796381 [details] [diff] [review]
part 3.  Add support for the noopener window feature in windowwatcher

Turns out as soon as I wrote some tests people decided the spec is wrong...  Canceling review for now; this needs to be totally redone.  :(

	Mike Conley (:mconley) (:⚙️)
	Comment 6 • 8 years ago

Comment on attachment 8796379 [details] [diff] [review]
part 1.  Remove the unused aOpenerFullZoom argument to nsPIWindowWatcher::OpenWindow2

Review of attachment 8796379 [details] [diff] [review]:
-----------------------------------------------------------------

Holy smokes, today I learned about Maybe.h. That thing is awesome.

	Mike Conley (:mconley) (:⚙️)
	Comment 7 • 8 years ago

Comment on attachment 8796380 [details] [diff] [review]
part 2.  Push maintenance of the popup spam count down into the window watcher

Review of attachment 8796380 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good overall, just a few minor points, mostly around naming choices. Thanks!

::: dom/base/nsGlobalWindow.cpp
@@ +1507,5 @@
> +  }
> +}
> +
> +void
> +nsGlobalWindow::SetPopupSpamWindow(bool aPopup)

I know that there are other examples of this aPopup arg being used for things like this, but I think this might be clearer if the bool was called "aIsPopupSpam" or something, since that's what this value really represents, I believe.

@@ +11857,5 @@
>    nsCOMPtr<nsPIWindowWatcher> pwwatch(do_QueryInterface(wwatch));
>    NS_ENSURE_STATE(pwwatch);
>  
> +  MOZ_ASSERT_IF(checkForPopup, abuseLevel < openAbused);
> +  bool isPopupSpamWindow = checkForPopup && (abuseLevel >= openControlled);

Not really an issue, but thought I'd call this out: At first glance, this looks a bit contradictory since we assert that abuseLevel < openAbused, but then check to see if abuseLevel >= openControlled (where the value above openControlled is openAbused).

I do appreciate, however, that this assertion is fatal in debug builds, and that we might want to err on the side of being defensive for opt builds.

You might want to consider putting in a short comment saying that abuseLevel at this point should definitely be less than openAbused, but that we're being intentionally defensive.

::: dom/base/nsGlobalWindow.h
@@ +1389,5 @@
>      return GetOuterWindowInternal()->mIsPopupSpam;
>    }
>  
> +  // Outer windows only.
> +  void SetPopupSpamWindow(bool aPopup);

Same as above - probably best to call it something like "aIsPopupSpam", to match the internal value it represents.

::: embedding/components/windowwatcher/nsPIWindowWatcher.idl
@@ +54,5 @@
>        @param aDialog use dialog defaults (see nsIDOMWindow::openDialog)
>        @param aNavigate true if we should navigate the new window to the
>               specified URL.
>        @param aArgs Window argument
> +      @parem aIsSpammyPopup true if the window is a popup spam window; used for

Nit: "parem" -> "param"

Note that we've got "IsPopupSpam" used elsewhere, so now we're introducing a second variation: "IsPopupSpam" and "IsSpammyPopup". This is minor, but might add cognitive drag while reading. Perhaps we should name this "aIsPopupSpam".

::: embedding/components/windowwatcher/nsWindowWatcher.cpp
@@ +1139,5 @@
>      if (newWindow) {
>        newWindow->SetInitialPrincipalToSubject();
> +      if (aIsSpammyPopup) {
> +        nsGlobalWindow* globalWin = nsGlobalWindow::Cast(newWindow);
> +        if (!globalWin->IsPopupSpamWindow()) { // Shouldn't this always be true?

Perhaps MOZ_ASSERT this for sanity?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 8 years ago

> but I think this might be clearer if the bool was called "aIsPopupSpam"

Yes, and the method called SetIsPopSpamWindow.  Done.

> At first glance, this looks a bit contradictory since we assert that
> abuseLevel < openAbused, but then check to see if
> abuseLevel >= openControlled (where the value above openControlled is openAbused).

I guess the question is why we don't just test for == openControlled, right?  I'll add some comments explaining that it's just defensiveness, yes.  Plus maybe we'll add some new popup level sometime... ;)

> Nit: "parem" -> "param"

Fixed.

> Perhaps we should name this "aIsPopupSpam".

Done.

> Perhaps MOZ_ASSERT this for sanity?

Done.

	Mike Conley (:mconley) (:⚙️)
	Comment 11 • 8 years ago

Comment on attachment 8801374 [details] [diff] [review]
part 3.  Add support for the noopener window feature in windowwatcher

Review of attachment 8801374 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks bz. I have some suggestions below for simplifications and documentation. I also have one or two questions I'm hoping can be cleared up before landing. Otherwise this seems pretty straight forward.

::: dom/base/nsGlobalWindow.cpp
@@ +6066,5 @@
>    NS_PRECONDITION(IsOuterWindow(), "Must be outer window");
>    NS_PRECONDITION(mDocShell, "Must have docshell");
>  
> +  if (aForceNoOpener) {
> +    if (!aName.LowerCaseEqualsLiteral("_self") &&

I think this can be simplified to:

if (aName.LowerCaseEqualsLiteral("_self") ||
    aName.LowerCaseEqualsLiteral("_top") ||
    aName.LowerCaseEqualsLiteral("_parent")) {
  return true;
}

return false;

or, even further:

return aName.LowerCaseEqualsLiteral("_self") ||
       aName.LowerCaseEqualsLiteral("_top") ||
       aName.LowerCaseEqualsLiteral("_parent");

can it not?

@@ +11845,5 @@
> +  // HTML whitespace trimming...
> +  nsCharSeparatedTokenizerTemplate<nsContentUtils::IsHTMLWhitespace> tok(
> +    aOptions, ',');
> +  while (tok.hasMoreTokens()) {
> +    if (tok.nextToken().EqualsLiteral("noopener")) {

Do we care about casing?

::: dom/webidl/Window.webidl
@@ +59,5 @@
>    [Throws, CrossOriginReadable] attribute any opener;
>    //[Throws] readonly attribute WindowProxy parent;
>    [Replaceable, Throws, CrossOriginReadable] readonly attribute WindowProxy? parent;
>    [Throws, NeedsSubjectPrincipal] readonly attribute Element? frameElement;
> +  //[Throws] WindowProxy? open(optional USVString url = "about:blank", optional DOMString target = "_blank", [TreatNullAs=EmptyString] optional DOMString features = "");

I can't say I really understand what's happening in here. As far as I can tell, some documentation is being changed, but I don't really see how it relates to this bug... would you be able to explain it to me? Or perhaps have DOM peer sign off on this part instead?

::: embedding/components/windowwatcher/nsWindowWatcher.cpp
@@ +1321,5 @@
>      }
>    }
>  
> +  if (aForceNoOpener && windowIsNew) {
> +    NS_RELEASE(*aResult);

Should we not be setting *aResult to nullptr as well to ensure that we're passing back a window if something else managed to take a reference to it while it was being opened? Or do you think this is unlikely?

::: testing/web-platform/tests/html/browsers/the-window-object/window-open-noopener.html
@@ +28,5 @@
> +    shouldReuse: false },
> +];
> +
> +var tests = [];
> +for (var i = 0; i < testData.length; ++i) {

I've figured out what this for-loop does after a bit of inspection, but to save time in the future, it'd probably be good to have some kind of docstring before it describing what it's doing with testData, and what kind of things it's testing for.

@@ +69,5 @@
> +
> +      w1 = window.open("", windowName);
> +      assert_equals(w1.opener, window);
> +
> +      var w2 = window.open("support/noopener-target.html?"+windowName,

Spaces on either side of + please

@@ +78,5 @@
> +    });
> +  }
> +}
> +
> +for (var target of ["_self", "_parent", "_top"]) {

Same as above re: docstring. Just something short describing the sorts of things this part of the test is testing for.

@@ +81,5 @@
> +
> +for (var target of ["_self", "_parent", "_top"]) {
> +  var t = async_test("noopener window.open targeting " + target);
> +  tests.push(t);
> +  t.openedWindow = window.open(`javascript:var w2 = window.open("", "${target}", "noopener"); this.checkValues(w2); this.close(); void(0);`);

Why the void, out of curiosity?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 8 years ago

> or, even further:

Good catch, will do.

> Do we care about casing?

Per spec, yes.  I'll add a test that verifies that using a different case doesn't get noopener behavior.

That said, the Chrome impl doesn't follow the spec (which they wrote!) here, in all sorts of ways including this one.  See <https://bugs.chromium.org/p/chromium/issues/detail?id=651596>.  So depending on how that gets resolved we may need to change behavior at some point.

> As far as I can tell, some documentation is being changed, but I don't really see how it relates to this bug

It really doesn't except insofar as I was reading the spec and reading our code while working on this bug and discovered that our "commented out copy of the spec IDL" wasn't actually a copy of the spec IDL.  So I adjusted it to be such a copy...  I could just take it out and land it in a separate commit with a comment to that effect; will do so.

> Should we not be setting *aResult to nullptr as well

NS_RELEASE(foo) includes "foo = 0" in the macro expansion.  Anything else would leave you with a dangling pointer.  ;)

> it'd probably be good to have some kind of docstring before it describing what it's doing with testData
> Spaces on either side of + please
> Same as above re: docstring. 

Will do all three.

> Why the void, out of curiosity?

To be clear that we are not trying to trigger navigation after the javascript: URL is evaluated here.  Probably redundant, since window.close() returns undefined anyway, but this makes it explicit.

	Pulsebot
	Comment 14 • 8 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae4239e6eab
part 1.  Remove the unused aOpenerFullZoom argument to nsPIWindowWatcher::OpenWindow2.  r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0d60789d0b
part 2.  Push maintenance of the popup spam count down into the window watcher.  r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d73ae4c960f
part 3.  Add support for the noopener window feature in windowwatcher.  r=mconley

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7ae4239e6eab
https://hg.mozilla.org/mozilla-central/rev/7f0d60789d0b
https://hg.mozilla.org/mozilla-central/rev/3d73ae4c960f

	Chris Mills [:cmills]
	Comment 16 • 7 years ago

I've documented this feature, adding an entry to the window features list on the Window.open() page:
https://developer.mozilla.org/en-US/docs/Web/API/Window/open#Window_functionality_features

I've also added a note to the Fx52 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM

Let me know if this looks OK. Thanks!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 7 years ago

Chris, I think one important missing bit is that if you use the "noopener" feature, window.open() will return null even if the window got opened.

That is not only can the new window not talk to you, but you can't talk to it either.

There are also complications around how named targets are handled when noopener is used, both here and for <a rel="noopener">: nonempty target names other than "_top", "_self", and "_parent" are all treated like "_blank" in terms of deciding whether to open a new window/tab.

	Chris Mills [:cmills]
	Comment 18 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> Chris, I think one important missing bit is that if you use the "noopener"
> feature, window.open() will return null even if the window got opened.
> 
> That is not only can the new window not talk to you, but you can't talk to
> it either.
> 
> There are also complications around how named targets are handled when
> noopener is used, both here and for <a rel="noopener">: nonempty target
> names other than "_top", "_self", and "_parent" are all treated like
> "_blank" in terms of deciding whether to open a new window/tab.

Thanks for the review Boris. I've added the required details to both pages.