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

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.

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.

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.

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.

Er, ccing Bejamin for comment 3. See bug 605999 for the right way to fix this sort of thing without going all XPCOM.

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

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.

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.

> We could disallow %{C++ within the interface body entirely Sold. Where does that bug belong?

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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)