614392 - attempt to set animationMode on imgIContainer results in a crash [@ mozilla::imagelib::Image::GetAnimationMode ]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Simon Kornblith]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101123 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101123 Firefox/4.0b8pre

The below code snippet is based on code inherited a long time ago from another project, but it seems to run without error in Firefox 3.6, whereas it completely crashes Firefox 4.

At present, I've only tested on OS X.

Reproducible: Always

Steps to Reproduce:
var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].  
	getService(Components.interfaces.nsIWindowMediator);  
var recentWindow = wm.getMostRecentWindow("navigator:browser");
var newTabBrowser = recentWindow.gBrowser.getBrowserForTab(
	recentWindow.gBrowser.addTab("http://www.google.com/"));  
newTabBrowser.addEventListener("load", function () {
	var aNode = newTabBrowser.contentDocument.getElementsByTagName("img")[0];
	var container = aNode
		.QueryInterface(Components.interfaces.nsIImageLoadingContent)
		.getRequest(Components.interfaces.nsIImageLoadingContent.CURRENT_REQUEST)
		.image;
	container.animationMode = Components.interfaces.imgIContainer.kDontAnimMode;
}, true);
Actual Results:  
Firefox crashes with EXC_BAD_ACCESS

Expected Results:  
Firefox disables animation on aNode

Crash ID 5031f20b-7f29-451c-8ad4-6a5542101123
http://crash-stats.mozilla.com/report/index/bp-5031f20b-7f29-451c-8ad4-6a5542101123

Comment 1

[Boris Zbarsky [:bzbarsky]]

This looks really broken.  The crash is at address 0x1, and the top of the stack is:

mozilla::imagelib::Image::GetAnimationMode modules/libpr0n/src/Image.cpp:139
NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208
XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:3042
XPC_WN_GetterSetter 	js/src/xpconnect/src/xpcprivate.h:2632
js::Invoke 	js/src/jscntxtinlines.h:684
js::ExternalInvoke 	js/src/jsinterp.cpp:858
js::JSProxyHandler::call 	js/src/jsproxy.cpp:248
JSCrossCompartmentWrapper::call 	js/src/jswrapper.cpp:235
js::proxy_Call 	js/src/jsproxy.cpp:795
js::Invoke 	js/src/jscntxtinlines.h:684
js::ExternalInvoke 	js/src/jsinterp.cpp:858
js::ExternalGetOrSet 	js/src/jsinterp.h:954
js::JSProxyHandler::set 	js/src/jscntxtinlines.h:746

So this is trying to call the setter, but lands in the getter, which sees 0x1 as the out param, tries to write to it, and bang.

Comment 2

[Boris Zbarsky [:bzbarsky]]

No, this is totally an imagelib bug.  imgIContainer has this gem:

109 %{C++
110   virtual PRUint16 GetType() = 0;
111 %}

which of course means the vtable it _actually_ has and the vtable xpconnect _thinks_ it has don't match.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is a regression from bug 584841.

Can we somehow make this construct just error out, if our srs aren't going to catch it?  This is at least the second time this mistake has been made in the 4.0 cycle, since we now seem to be happy to sprinkle non-COM stuff in our IDL willy-nilly.

Comment 4

[Daniel Holbert [:dholbert]]

Specifically, it's from bug 584841 Comment 25 (after discussion between me & bholley in IRC about tradeoffs with XPCOM purity, outparam-hell, & easy-assertability).

We can easily convert GetType into an XPCOM read-only attr to fix this, though. I'll do that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er, ccing Bejamin for comment 3.

See bug 605999 for the right way to fix this sort of thing without going all XPCOM.

Comment 6

[Boris Zbarsky [:bzbarsky]]

In this case, you'd want [notxpcom] too.

Comment 7

[Daniel Holbert [:dholbert]]

We actually already have a [yesxpcom] read-only variable for this (in case scripts care about what type of image they're dealing with) -- the %C++{%} GetType method was just a convenience method.

So, I think I just need to delete the %C++{%} block and update all the callers to use the existing XPCOM-style getter.

Comment 8

[Benjamin Smedberg]

We could disallow %{C++ within the interface body entirely, which would screw a few things up but not many. I don't think there's any other way to statically check that kind of correctness.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> We could disallow %{C++ within the interface body entirely

Sold.  Where does that bug belong?

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix

Still need to add a test, but here's the fix.

Comment 11

[Boris Zbarsky [:bzbarsky]]

The problem is that this will break C++ extensions that use the interface (at least requiring a recompile).  That's where [notxpcom] would get us the best of both worlds.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 495127 [details] [diff] [review]
fix

For Image, I'd rather you do

PRUint16 GetType()
{
  PRUint16 type;
  GetType(&type);
  return type;
}

using Image::GetType;

and remove the implementations of GetType in the concrete classes.

I'm ok with it if you decide that's not a good idea because of the theoretical possibility of GetType throwing, but at least do the "using" instead of redeclaring GetType from Image.

Comment 13

[Joe Drew (not getting mail)]

Frig, I forgot about binary compatibility. We could do the [notxpcom] bit now, and change the rest in 2.next, I suppose.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase (needs privs, so only works locally)

Here's a reduced standalone testcase that reproduces this, when saved locally.

(It needs privileges, so I'll convert it into a mochitest for adding to our test suite.)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (add [notxpcom]), with mochitest

Ok, here's a fix with just a switch to [notxpcom].

Requesting sr to be on the safe side, since this is a tweak to an interface (though it has no repercussions for users of the interface, AFAIK).

Mochitest included -- I verified that it crashes pre-patch but passes post-patch.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 495188 [details] [diff] [review]
fix v2 (add [notxpcom]), with mochitest

oh, I should probably rev the UUID, too, since the vtable is slightly different now.  Canceling review, new patch coming in a sec...

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: fix v2b: [notxpcom] w/ uuid rev, & mochitest

actually, maybe the UUID rev is one of the things we're trying to avoid the need for, per comment 11?

In any case, here's a version with the UUID rev, and I can drop it if we don't need/want it. :)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 495190 [details] [diff] [review]
fix v2b: [notxpcom] w/ uuid rev, & mochitest

I think you want [noscript, notxpcom] and you do _not_ want to change the IID.

sr=me with those two nits.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: fix v2c: [noscript, notxpcom] & mochitest (sr=bz)

ok, thanks bz -- here's the patch again with that fixed.

Comment 20

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/cdb7dae6fdbb

Comment 21

[Daniel Holbert [:dholbert]]

This patch busted windows builds -- looks like I need to tweak the signature of GetType() in order for Windows to like it.

Backed out for now, & will land a fixed patch after I've sanity-checked it on windows tryserver.

Comment 22

[Daniel Holbert [:dholbert]]

(backout was http://hg.mozilla.org/mozilla-central/rev/e24641018d51 )

Comment 23

[[e-mail]]

You probably need to change virtual PRUint16 into NS_IMETHOD_(PRUint16).

Comment 24

[Daniel Holbert [:dholbert]]

(Yup, agreed -- and as long as I'm doing that, I'm also moving that decl up to the NS_DECL_IMGICONTAINER expanded block and un-inlining it, to keep that block of code "pure" (i.e. straight from the auto-generated imgIContainer.h file))

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: followup: fix concrete classes to use NS_IMETHOD_ for GetType decl

Here's the followup to use "NS_IMETHOD_" on GetType() in the concrete classes RasterImage & VectorImage.

I replaced the old decls with auto-generated one copied straight from imgIContainer.h, and I un-inlined it for cleanliness' sake.

Note that now VectorImage and RasterImage have identical (3-line) GetType(PRUint16* ) impls.  I initially tried merging those into an inherited method on Image, but that ended up being problematic because it causes build-warning-spam (about e.g. Image::GetType(PRUint16*) being hidden by RasterImage::GetType()).  I couldn't immediately find a clean way to fix that warning, so I ended up just keeping the impls in the base classes for now, for simplicity's sake.

Comment 26

[Joe Drew (not getting mail)]

Comment on attachment 495660 [details] [diff] [review]
followup: fix concrete classes to use NS_IMETHOD_ for GetType decl

I presume this has passed through try? And the NS_IMETHODIMP_() was what was broken on Windows?

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to comment #26)
> I presume this has passed through try?

It's running through try at the moment (on all platforms).  I'll make sure it passes before landing.

> And the NS_IMETHODIMP_() was what was
> broken on Windows?

Yes, I think so. The error looked like this:
{
e:\builds\moz2_slave\mozilla-central-win32\build\modules\libpr0n\src\RasterImage.h(192) : error C2695: 'mozilla::imagelib::RasterImage::GetType': overriding virtual function differs from 'imgIContainer::GetType' only by calling convention
e:\builds\moz2_slave\mozilla-central-win32\build\obj-firefox\dist\include\imgIContainer.h(81) : see declaration of 'imgIContainer::GetType'
}

Comment 28

[Daniel Holbert [:dholbert]]

The try run has completed at least one build on each platform, and they're all green, so I think this is good to go once it gets sr=bz!

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 495660 [details] [diff] [review]
followup: fix concrete classes to use NS_IMETHOD_ for GetType decl

Nix the stray space between '6' and ')' in both places, and looks good.

Comment 30

[Daniel Holbert [:dholbert]]

(FWIW, that stray space is from the NS_DECL_IMGICONTAINER chunk in imgIContainer's auto-generated .h file - I thought it was odd, but I kept it for the sake of exactly matching the macro that I'm expanding.  Glad to remove it, though. :))

Comment 31

[Daniel Holbert [:dholbert]]

Re-landed:
 original patch: http://hg.mozilla.org/mozilla-central/rev/ec43a3a73f80
 followup:       http://hg.mozilla.org/mozilla-central/rev/4955ecee83e7

Comment 32

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)