Closed Bug 868996 Opened 7 years ago Closed 7 years ago

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

Categories

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

20 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox20 --- wontfix
firefox21 + wontfix
firefox22 + verified
firefox23 + verified

People

(Reporter: petkoc, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
Flags: needinfo?(petkoc)
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.
Flags: needinfo?(petkoc)
The javascript error reported by the FF WebConsole is "TypeError: opener is null @ http://demo-admin.magentocommerce.com/js/mage/adminhtml/browser.js:232
Assignee: nobody → general
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Summary: Magento Insert Image button don't work any more. → Magento Insert Image button don't work any more. JavaScript TypeError: opener is null
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.
Blocks: 823283
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
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...
Assignee: general → bzbarsky
Whiteboard: [need review]
Component: JavaScript Engine → DOM
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).
Attachment #746982 - Flags: review?(bobbyholley+bmo) → review-
> 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.
Attachment #747084 - Flags: review?(bobbyholley+bmo)
Attachment #746982 - Attachment is obsolete: true
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
Attachment #747084 - Flags: review?(bobbyholley+bmo) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5dee705c5e
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla23
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.
Attachment #747084 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d0829a642fd9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #747084 - Flags: approval-mozilla-aurora?
Attachment #747949 - Flags: approval-mozilla-aurora?
Attachment #747949 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 747949 [details] [diff] [review]
Patch merged to Aurora

Approving for Beta 22
Attachment #747949 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.