888746 - browser_bug422590.js shouldn't use chrome-privileged window.open with toolbar=no

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[:Gijs (he/him)]

STR:

1. Open the browser console.
2. Run: window.open("http://www.mozilla.org", "_blank", "toolbar=no");

ER:
A window opens with no toolbar showing mozilla.org, and no errors occur.

AR:
A window opens with no toolbar showing mozilla.org, and this error appears in the console:

[19:35:57.146] this.editor is null @ chrome://browser/content/urlbarBindings.xml:161

The relevant line is:

          let controller = this.editor.selectionController;

in formatValue.

Patch coming up.

(we ran into this on Australis, but it happens on mozilla-central, too)

Comment 1

[:Gijs (he/him)]

Attachment: Trivial wallpaper

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 769504 [details] [diff] [review]
Trivial wallpaper

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

r=me assuming tests pass.

Comment 3

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #2)
> Comment on attachment 769504 [details] [diff] [review]
> Trivial wallpaper
> 
> Review of attachment 769504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me assuming tests pass.

https://tbpl.mozilla.org/?tree=Try&rev=a76c0a0af79f

Accidentally pushed w/ UX + Jared's patch to lazily load stuff (without the backout; tbpl is misleading). :-\

Do you want another m-c-based try run?

Comment 4

[Matthew N. [:MattN]]

That's probably fine. I just wanted to make sure this didn't somehow regress other tests.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Yes, I came to the same conclusion today but didn't get it on to Bugzilla quick enough! This is a problem that has existed for a while, and I ran in to it with bug 793715 (https://hg.mozilla.org/mozilla-central/rev/bf0aa600323f).

The deeper explanation of the test failing now is because we removed the secondary TabSelect and ProgressListener. The exception is happening previously (it's still there even after the backout) but the other onLocationChange caused us to still enter customization mode in the chromeless window. Now that the CustomizeMode change got rid of the second progress listener, when it fails it doesn't have a second chance to pick itself back up.

Comment 6

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/7965d384123c

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 769504 [details] [diff] [review]
Trivial wallpaper

You should fix the test. We don't allow hiding the navigation toolbar and this.editor should never be null here. When you enter window.open("http://www.mozilla.org", "_blank", "toolbar=no"); in the browser console, you're running it with chrome privileges.

Comment 8

[Dão Gottwald [:dao]]

backed out:

https://hg.mozilla.org/mozilla-central/rev/35f1ddf84221
https://hg.mozilla.org/mozilla-central/rev/5e66712edb18

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 769504 [details] [diff] [review]
> Trivial wallpaper
> 
> You should fix the test. We don't allow hiding the navigation toolbar and
> this.editor should never be null here. When you enter
> window.open("http://www.mozilla.org", "_blank", "toolbar=no"); in the
> browser console, you're running it with chrome privileges.

Right, and so is the test. I don't know of a way to make that call run in-content from the mochitest's test script.

Additionally, I expect window.open has other chrome consumers (in add-ons or our own code) for whom breaking the onlocationchange function is harmful.

Comment 10

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #8)
> backed out:
> 
> https://hg.mozilla.org/mozilla-central/rev/35f1ddf84221
> https://hg.mozilla.org/mozilla-central/rev/5e66712edb18

Next time you back out, please use hg backout so you don't need a separate merge changeset, and reopen the bug.

Comment 11

[:Gijs (he/him)]

So regarding the test... this sort of works, assuming window.content is actually not privileged:

let sandbox = Cu.Sandbox(window.content);
sandbox.window = window.content;
Cu.evalInSandbox("window.open('about:blank', '_blank', 'chrome,toolbar=no')", sandbox);


... except then the popup blocker screws us over and we don't get a new window. Ideas?

Comment 12

[Dão Gottwald [:dao]]

You could either disable the popup blocker or keep the call privileged and add location=yes.

Comment 13

[:Gijs (he/him)]

Attachment: Fix test instead

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/ffbdd1090cb8