335731 - Crash while calling HTMLElement.prototype.toString()

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I noticed that calling HTMLElement.prototype.toString() crashes the other day. This is fallout from bug 334104, where we added a call to XPCCallContext::SetName from XPC_WN_Shared_ToString. The problem is that SetName assumes that mWrapper is non-null, but HTMLElement.prototype is of class sDOMConstructorProtoClass, which is not a wrapped native.

I'm not entirely sure of what the proper fix is, yet. This is a null pointer dereference, and therefore I'm not marking this security sensitive.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Since bug 334104 was checked into the branch, this should probably block 1.8.0.4.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This adds null checks. We have to be able to handle the mWrapper == null case, since that happens whenever we're operating on a wrapped native prototype.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 220071 [details] [diff] [review]
Fix

> XPCCallContext::GetCallee(nsISupports * *aCallee)
> {
>-    nsISupports* temp = mWrapper->GetIdentityObject();
>+    nsISupports* temp = mWrapper ? mWrapper->GetIdentityObject() : nsnull;
>     NS_IF_ADDREF(temp);
>     *aCallee = temp;
>     return NS_OK;

Is it OK to return success if the callee is null? If it's anything like QI callers might expect a failure (though they're probably checking for null). Same in the getClassInfo below.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #3)
> Is it OK to return success if the callee is null? If it's anything like QI
> callers might expect a failure (though they're probably checking for null).
> Same in the getClassInfo below.

It seems that all of the users of GetCallee in the tree either ignore the return value or check for null anyway. GetCalleeWrapper is expected to return non-null if it returns a success code, though. I guess I could make the functions throw if they're going to return null, though I'm not sure if that's really the correct behavior...

Comment 5

[chris hofmann]

be good to get for 1.9a1 too.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 220071 [details] [diff] [review]
Fix

r+sr=jst

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 220071 [details] [diff] [review]
Fix

This patch fixes a crash regression introduced by a bug that was already checked into the branches.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 220071 [details] [diff] [review]
Fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 11

[Chris Cooper [:coop] (he/him)]

Attachment: Testcase: simple call to HTMLElement.prototype.toString()

Comment 12

[Chris Cooper [:coop] (he/him)]

Testcase passes (no crash), and verified by code inspection.