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

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Simon Kornblith, Assigned: dholbert)

Tracking

Trunk
mozilla2.0b8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
Version: unspecified → Trunk
Component: Graphics → ImageLib
QA Contact: thebes → imagelib
Summary: attempt to set animationMode on imgIContainer results in a crash → attempt to set animationMode on imgIContainer results in a crash [@ mozilla::imagelib::Image::GetAnimationMode ]
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.
Status: UNCONFIRMED → NEW
Component: ImageLib → XPConnect
Ever confirmed: true
QA Contact: imagelib → xpconnect
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.
Component: XPConnect → ImageLib
QA Contact: xpconnect → imagelib
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.
blocking2.0: --- → beta9+
(Assignee)

Comment 4

8 years ago
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 7

8 years ago
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

8 years ago
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?
(Assignee)

Comment 10

8 years ago
Created attachment 495127 [details] [diff] [review]
fix

Still need to add a test, but here's the fix.
Attachment #495127 - Flags: review?(joe)
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.
Attachment #495127 - Flags: review?(joe) → review+
Frig, I forgot about binary compatibility. We could do the [notxpcom] bit now, and change the rest in 2.next, I suppose.
(Assignee)

Comment 14

8 years ago
Created attachment 495155 [details]
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.)
(Assignee)

Updated

8 years ago
Attachment #495155 - Attachment is patch: false
Attachment #495155 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 15

8 years ago
Created attachment 495188 [details] [diff] [review]
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.
Attachment #495188 - Flags: superreview?(bzbarsky)
Attachment #495188 - Flags: review?(joe)
(Assignee)

Comment 16

8 years ago
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...
Attachment #495188 - Flags: superreview?(bzbarsky)
Attachment #495188 - Flags: review?(joe)
(Assignee)

Comment 17

8 years ago
Created attachment 495190 [details] [diff] [review]
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. :)
Attachment #495190 - Flags: superreview?(bzbarsky)
Attachment #495190 - Flags: review?(joe)
(Assignee)

Updated

8 years ago
Attachment #495127 - Attachment is obsolete: true
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.
Attachment #495190 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 19

8 years ago
Created attachment 495411 [details] [diff] [review]
fix v2c: [noscript, notxpcom] & mochitest (sr=bz)

ok, thanks bz -- here's the patch again with that fixed.
Attachment #495188 - Attachment is obsolete: true
Attachment #495190 - Attachment is obsolete: true
Attachment #495411 - Flags: superreview+
Attachment #495411 - Flags: review?(joe)
Attachment #495190 - Flags: review?(joe)
(Assignee)

Updated

8 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Attachment #495411 - Flags: review?(joe) → review+
(Assignee)

Comment 20

8 years ago
http://hg.mozilla.org/mozilla-central/rev/cdb7dae6fdbb
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
(Assignee)

Comment 21

8 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You probably need to change virtual PRUint16 into NS_IMETHOD_(PRUint16).
(Assignee)

Comment 24

8 years ago
(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))
(Assignee)

Comment 25

8 years ago
Created attachment 495660 [details] [diff] [review]
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.
Attachment #495660 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #495660 - Flags: review? → review?(joe)
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?
Attachment #495660 - Flags: superreview?(bzbarsky)
Attachment #495660 - Flags: review?(joe)
Attachment #495660 - Flags: review+
(Assignee)

Comment 27

8 years ago
(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'
}
(Assignee)

Comment 28

8 years ago
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!
Whiteboard: [awaiting sr]
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.
Attachment #495660 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 30

8 years ago
(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. :))
(Assignee)

Updated

8 years ago
Whiteboard: [awaiting sr]
(Assignee)

Comment 31

8 years ago
Re-landed:
 original patch: http://hg.mozilla.org/mozilla-central/rev/ec43a3a73f80
 followup:       http://hg.mozilla.org/mozilla-central/rev/4955ecee83e7
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Comment 32

8 years ago
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)
No longer blocks: 584841
blocking2.0: beta9+ → betaN+
Blocks: 584841
You need to log in before you can comment on or make changes to this bug.