868996 - Magento Insert Image button don't work any more. JavaScript TypeError: opener is null

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Petko]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130326150557

Steps to reproduce:

Upgrade from 19.0.2 to version 20.0.0


Actual results:

In the backend, when you go to insert a image via the WYSIWYG editor, the insert image button no longer works.
You can verify the issue via the original magento demo backend. Please visit http://demo-admin.magentocommerce.com/index.php/admin/ and after go to CMS > Pages. You can edit an existing page or create a new. Please use the WYSIWYG editor to insert image.


Expected results:

Insert Image button must work like with branch 19.

Comment 1

[Loic]

Do you mean this button to insert image?
http://i.imgur.com/kFUkOzX.jpg

Comment 2

[Petko]

Hi Loic
there is one more step to do.

1- After your screen you must press the button like in this image http://i.imgur.com/FBdCvXR.jpg

2- then, look how to insert the image in the second screen. http://i.imgur.com/S5n8R8H.jpg

3- Normally, once you press the button, the window in the point 1, will have a link.

Forgive my bad English.

Comment 3

[Tiziana Sellitto [:tiziana]]

The javascript error reported by the FF WebConsole is "TypeError: opener is null @ http://demo-admin.magentocommerce.com/js/mage/adminhtml/browser.js:232

Comment 4

[Boris Zbarsky [:bzbarsky]]

Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bf26f61a0748&tochange=84320dffec6e

My money is on bug 823283 here, since the throwing function looks like this:

    getTargetElement: function() {
        if (typeof(tinyMCE) != 'undefined' && tinyMCE.get(this.targetElementId)) {
            if ((opener = this.getMediaBrowserOpener())) {
                var targetElementId = tinyMceEditors.get(this.targetElementId).getMediaBrowserTargetElementId();
                return opener.document.getElementById(targetElementId);

note the attempt to set "opener", which nowadays does nothing but used to reconfigure the property...

What we do now is the HTML5 spec says, but I just tested, and this script:

    opener = 20; alert(opener);

alerts 20 in Firefox 19, Chrome, Safari, Opera, and IE9.  This one:

  opener = window; alert(opener);

alerts the window in older IE versions too...

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=21946 on the spec, but it seems like we need to change the behavior on our end somehow here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, so I'm going to take the following approach.  When we're doing an unprivileged set of opener to a non-null opener, I will simply define the opener property directly on the window.  Need to write some tests...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow setting opener on the window to non-null, for just the lifetime of the page.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 746982 [details] [diff] [review]
Allow setting opener on the window to non-null, for just the lifetime of the page.

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

::: dom/base/nsGlobalWindow.cpp
@@ +3877,5 @@
>  NS_IMETHODIMP
>  nsGlobalWindow::SetOpener(nsIDOMWindow* aOpener)
>  {
> +  // check if we were called from a privileged chrome script.  If not, and if
> +  // aOpener is not null, just define aOpener on our inner window's JS object,

(mention the Xray possibility here)

@@ +3890,5 @@
> +
> +    nsIScriptContext *scx = GetContextInternal();
> +    NS_ENSURE_TRUE(scx, NS_ERROR_NOT_INITIALIZED);
> +
> +    JSContext *cx = scx->GetNativeContext();

Instead of all this, please just do:

AutoJSContext cx;

@@ +3892,5 @@
> +    NS_ENSURE_TRUE(scx, NS_ERROR_NOT_INITIALIZED);
> +
> +    JSContext *cx = scx->GetNativeContext();
> +
> +    JSAutoCompartment ac(cx, mJSObject);

As discussed on IRC, instead of entering the compartment of the window, let's just wrap the window into the caller's compartment so that we do the right thing for Xrays. Please add a comment to that effect as well.

Note that this will cause |opener| to end up on the expando object (per origin) rather than the holder (per wrapper). But I believe that's what we want here.

@@ +3893,5 @@
> +
> +    JSContext *cx = scx->GetNativeContext();
> +
> +    JSAutoCompartment ac(cx, mJSObject);
> +    JSAutoRequest ar(cx);

No need for the ar here, I don't think.

::: dom/base/test/test_setting_opener.html
@@ +13,5 @@
> +  /** Test for Bug 868996 **/
> +  SimpleTest.waitForExplicitFinish();
> +
> +  function testOpenerSet() {
> +    window.open("data:text/html,<script>opener.basicOpenerTest(this)</" + "script>");

What's the string break here about? Are you trying to fool some kind of XSS detector?

@@ +19,5 @@
> +
> +  function basicOpenerTest(win) {
> +    is(win.opener, window, "Opening a window should give it the right opener");
> +    win.opener = $("x").contentWindow;
> +    is(win.opener, $("x").contentWindow, "Should be able to set an opener to a different window");

Please add tests here for a sandbox with a principal of |win| with {wantXrays: true} and sandbox with |[win]| (an nsEP) to make sure that each gets its own view on opener. And make sure to test that these things are appropriately per-inner (which they should be, since the expando object hangs off the inner).

Comment 8

[Boris Zbarsky [:bzbarsky]]

> AutoJSContext cx;

OK.

> No need for the ar here, I don't think.

Why not?  What if we're called with no script on the stack?

> What's the string break here about? Are you trying to fool some kind of XSS detector?

This code is inside a <script>. Without the string break, the HTML parser would treat the </script> inside the string as closing the <script> the code is in, which is not desirable.

> Please add tests here for a sandbox

Hmm.  I'll give that a shot.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> > No need for the ar here, I don't think.
> 
> Why not?  What if we're called with no script on the stack?

Actually, you're right. We'll default to the SafeJSContext, but currently that won't enter a request. I'm fixing that soon in bug 868130, but for now the ar is fine.

> 
> > What's the string break here about? Are you trying to fool some kind of XSS detector?
> 
> This code is inside a <script>. Without the string break, the HTML parser
> would treat the </script> inside the string as closing the <script> the code is
> in, which is not desirable.

Ah, interesting! I didn't realize that the HTML parser wasn't script string aware.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: With review comments addressed

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 747084 [details] [diff] [review]
With review comments addressed

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

Nice. r=bholley with comments addressed.

::: dom/base/nsGlobalWindow.cpp
@@ +3879,5 @@
>  {
> +  // Check if we were called from a privileged chrome script.  If not, and if
> +  // aOpener is not null, just define aOpener on our inner window's JS object,
> +  // wapped into the current compartment so that for Xrays we define on the Xray
> +  // expando, but don't set it on the outer window, so that it'll get reset on

expando object. Also, this sentence could probably be split in two.

@@ +3888,5 @@
> +    NS_ENSURE_STATE(piWin);
> +
> +    nsCOMPtr<nsIGlobalObject> glob = do_QueryInterface(piWin->GetOuterWindow());
> +    NS_ENSURE_STATE(glob);
> +

Can you just QI aOpener directly to nsIGlobalObject? It should never be an inner if invoked from script, and even if it is, it'll get outerized in the WrapObject call anyway.

::: dom/base/test/test_setting_opener.html
@@ +14,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var sb1, sb2;
> +  var Cu = SpecialPowers.Cu;
> +  

Nit - whitespace.

@@ +21,5 @@
> +    var win = window.open("data:text/html,<script>opener.setTimeout(opener.basicOpenerTest, 0, this)</" + "script>");
> +    // A sandbox for the window
> +    sb1 = new Cu.Sandbox(win, {wantXrays: true, sandboxPrototype: win })
> +    // And a sandbox using the expanded principal.
> +    sb2 = new Cu.Sandbox([win], {wantXrays: true, sandboxPrototype: win })

I prefer to avoid using sandboxPrototype (and all of its magical baggage) in situations where we just need to give the sandbox a reference to a window. Can you just define |win| on the sandbox globals instead?

@@ +80,5 @@
> +    is(win.opener, null, "Null opener should persist across navigations");
> +    is(evalsb("window.opener", sb1), null,
> +       "Null opener should persist across navigations in sb1");
> +    is(evalsb("window.opener", sb2), null,
> +       "Null opener should persist across navigations in sb1");

sb2

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Can you just QI aOpener directly to nsIGlobalObject? 

I could.  I guess given that WrapObject outerizes it's ok to do that.

> Can you just define |win| on the sandbox globals instead?

Ah, nice.  Done.

> sb2

Fixed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5dee705c5e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for PGO bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0871f59b1ad5

https://tbpl.mozilla.org/php/getParsedLog.php?id=22789385&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22789466&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22789791&tree=Mozilla-Inbound

Comment 15

[Boris Zbarsky [:bzbarsky]]

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0829a642fd9

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 747084 [details] [diff] [review]
With review comments addressed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 823283 
User impact if declined: Some sites (e.g. the Magento CMS here) are broken
Testing completed (on m-c, etc.): Passes tests and manual testing
Risk to taking this patch (and alternatives if risky): Risk is fairly low; this
   restores the behavior we had in Firefox 19, as long as opener is being set to
   a Window.
String or IDL/UUID changes made by this patch: None.

Comment 17

[Ed Morley [:emorley]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch merged to Aurora

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 747949 [details] [diff] [review]
Patch merged to Aurora

Approving for Beta 22

Comment 20

[Tiziana Sellitto [:tiziana]]

Verified fixed with the latest mozilla central, on Mac OSX 10.8.3

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130513 Firefox/23.0
Build ID: 20130513031057

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/793614d152b1

Comment 22

[Tiziana Sellitto [:tiziana]]

Verified fixed on Mac OSX 10.8.3

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0
Build ID: 20130521223249