604262 - Crash [@ nsImageLoadingContent::OnStartDecode]

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Mounir Lamouri (:mounir)]

It looks like this does not crash on Firefox 3.6. Should that be a blocker?

Comment 2

[Johnny Stenback (:jst)]

Attachment: Fix.

This makes it so that only chrome can call methods on the interfaces that are not explicitly listed in classinfo for input elements.

Comment 3

[Johnny Stenback (:jst)]

Blocking.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 483355 [details] [diff] [review]
Fix.

If we're able to restrict access to Components.interfaces for FF4, then we should back this out.

r=me for now though.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is fixed by Bug 605271.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Bug 605271 isn't going to make it.  We should land this on trunk.

Comment 7

[Mounir Lamouri (:mounir)]

I was thinking to push this but it doesn't pass try and fails a lot of tests. One of them is content/base/test/test_copyimage.html

For more information, you can see the try results here:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=758292a1efa8

Comment 8

[Johnny Stenback (:jst)]

Attachment: Fix test failures.

So this got nasty. What happens here is that we have a bunch of code that ends up running from within untrusted JS, and that code calls into the image code to do whatever, and we check to see if the caller is trusted, but we find the untrusted JS code, and deny the C++ code from doing what it needs to do. This patch adds the (in)appropriate nsCxPusher's to push a null context where needed to hide the fact that this code was called from untrusted JS so that the C++ code can do what it needs to do. This really sucks, but I don't think we have a better option here.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 507741 [details] [diff] [review]
Fix test failures.

>@@ -145,7 +145,13 @@ nsSVGImageFrame::~nsSVGImageFrame()
>   if (mListener) {
>     nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mContent);
>     if (imageLoader) {
>-      imageLoader->RemoveObserver(mListener);
>+      // Push a null JSContext on the stack so that code that runs
>+      // within the below code doesn't think it's being called by
>+      // JS. See bug 604262.
>+      nsCxPusher pusher;
>+      pusher.PushNull();
>+ 
>+     imageLoader->RemoveObserver(mListener);

There's a whitespace issue on the last line here.

r=me with that fixed.

Comment 10

[(no longer active)]

This patch landed as:

http://hg.mozilla.org/mozilla-central/rev/4beb0e6ba654

Comment 11

[Ben]

Tested

Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415

does not crash when loading attachment.

4.0b10 did crash ! So it seems problem is solved.

Comment 12

[[:Aleksej]]

Verified on Linux, too — with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre  (crashed with 20110124030332)

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The patch landed, just looks like the bug was never closed.  Thanks for catching that Ben and Aleksej!

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Landed a test.

http://hg.mozilla.org/mozilla-central/rev/3341241b6ee0

Comment 15

[LegNeato]

Deferring to a future point release as we have run out of time. If this absolutely needs to be fixed and can land today or tomorrow, please send a note to [e-mail]

Comment 16

[Daniel Veditz [:dveditz]]

We still do need this one

Comment 17

[Johnny Stenback (:jst)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/47d90ae717a7

Comment 18

[Eiichi]

This landing seems to kill JS "var ButtonImage = new Array()" function.

Comment 19

[Eiichi]

Sorry for my insufficient explanation.
After changes pushed with changeset a7a241b581ad, 'onmouseover' and 'onmouseout' function don't work.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can you file a new bug with a testcase and CC me?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix more bustage on the 1.9.2 branch

This turned the 1.9.2 reftest tinderboxen orange because of sync/async differences between trunk and branch. This patch should make them go green. It has r=bz and a=dveditz (both in person).

Comment 22

[Daniel Veditz [:dveditz]]

bustage fix check-ins

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9ce9a35604f2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d8a491705d60

Comment 23

[Johnny Stenback (:jst)]

Attachment: Fix for 1.9.1

This is a combined patch against 1.9.1 of all that was checked in for 1.9.2. I found out that nsCxPusher::PushNull doesn't exist on the 1.9.1 branch, and adding it isn't trivial, so I did the pushing and popping by hand instead. And some of the changed code didn't exist on 1.9.1 so there are a couple of hunks that are different here.

Comment 24

[Daniel Veditz [:dveditz]]

(In reply to comment #20)
> Can you file a new bug with a testcase and CC me?

bug 649326 was filed on this, but was fixed by the patch in comment 21.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 526410 [details] [diff] [review]
Fix for 1.9.1

Looks good. I was wondering if it might have been worth it to add a nsNullCxPusher class, but it only helps one case added in this patch, so probably not.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 526410 [details] [diff] [review]
Fix for 1.9.1

Approved for 1.9.1.19, a=dveditz for release-drivers

Please land ASAP so we can re-spin today.

Comment 27

[Mounir Lamouri (:mounir)]

Pushed in 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a846a6d5bbf8

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

And to the relbranch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6a8fcd90b366

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I checked this with the attached testcase in build 1 (before the fix on 4/20) of 1.9.1.19. I do not see a crash. This is on Windows 7.

How does one cause the crash?

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Alternative testcase for old branch

Al, you can use this testcase on 1.9.1.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(It won't crash, but it should be enough to verify the fix here.)