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.

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

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.

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

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.

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 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).

> 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.

(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 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

> 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.

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

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

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 on attachment 747949 [details] [diff] [review] Patch merged to Aurora Approving for Beta 22

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

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