169321 - Passing strings generated by .match or .substr into Components.lookupMethod() will throw an exception.

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Using linux 2002091704:

Components.lookupMethod(window, 'alert'); // works fine
Components.lookupMethod(window, 'alert'.substr(0)); // doesn't work

in nsXPCComponents::LookupMethod() we are failing somewhere around here:

    // this will do verification and the method lookup for us
    XPCCallContext inner_cc(JS_CALLER, cx, obj, nsnull, argv[1]);

    // was our jsobject really a wrapped native at all?
    XPCWrappedNative* wrapper = inner_cc.GetWrapper();
    if(!wrapper || !wrapper->IsValid())
        return NS_ERROR_XPC_BAD_CONVERT_JS;


I have a testcase which needs to be loaded as chrome to work properly.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Testcase

This needs chrome priveleges to run properly.  Click on each of the three
buttons.  You should get no exceptions thrown.

Comment 2

[Johnny Stenback (:jst)]

After talking this over with Brendan I think we know what the deail is. I
realized that XPConnect does jsval compares when looking for names in the
wrappers n' what not and that means that the strings that are passed to the code
that does that lookup must be atomized strings, the results from .match() et al
are not atomized. Should be trivial to fix in XPConnect.

Comment 3

[jag (Peter Annema)]

Heh, I just came to the same conclusion after wading through exotic JS and
xpconnect code.

Comment 4

[Brendan Eich [:brendan]]

I'd say don't intern the name parameter to Components.lookupMethod, rather just
do string compares if the jsvals don't match.

/be

Comment 5

[Johnny Stenback (:jst)]

Attachment: This should fix the problem by atomizing the name.

Is there a cleaner way to do this in XPConnect? I don't know...

Comment 6

[Brendan Eich [:brendan]]

Attachment: alterna-patch that avoids creating lots of garbage atoms that are pinned forever

I'm not happy with this one, though -- and not just cuz it doesn't compile! (I
didn't bother to get the #includes right).  Because other callers funnel down
to XPCNativeInterface::FindMember, I'm concerned that a patch of this sort will
hurt all callers who *do* ensure uniquely associated string jsvals, and will
even ding app performance.

Can't we make nsXPCComponents::LookupMethod pay the price, and only that method
pay the price, of having to do strcmp if the jsval match fails?

/be

Comment 7

[Brendan Eich [:brendan]]

Of course, the lots of garbage atoms case arises only if the name parameter
doesn't match an existing interface's method.  Right?  Or do we lazily load
interface methods and intern their name strings only on demand, or some such?  I
could see jst's patch making things no worse in the good case, where the name
does match.  But in the mismatch case, it would seem to pin garbage atoms.

/be

Comment 8

[Brendan Eich [:brendan]]

BTW, there's a workaround for this bug.  Try using the computed string (the
substr() return value, e.g.) as an index into a dummy object.  Then you should
be able to use that same string value (you'll have to save the r.v. or whatever
the result of the computation is in a variable) as the name parameter to
Components.lookupMethod.

/be

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 99614 [details] [diff] [review]
alterna-patch that avoids creating lots of garbage atoms that are pinned forever

I don't think this is acceptable for performance reasons (as brendan already
mentioned), and I don't know the XPConnect code well enough if we can easily
make Components.lookupMethod() pay the price here w/o atimizing the strings
that come through it.

Brendan, does the JS_ValueToId() call always intern the string? What I really
want is something like JS_StringToAtimzedValue(), can't that be done w/o
pinning the strings for the life of the runtime?

Comment 10

[Johnny Stenback (:jst)]

Damn, my finger's don't work today, s/atimize/atomize/g, in all 3 places (at
least), geez!

Comment 11

[Brendan Eich [:brendan]]

Argh, I am losing it.  JS_ValueToId does not pin the atom for the life of the
runtime, as JS_InternString does.  I should have read jst's patch.  It's good to
go.  I'll invalidate mine.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 99613 [details] [diff] [review]
This should fix the problem by atomizing the name.

The only issue is that if the GC could nest somewhere after the JS_ValueToId
and JS_IdToValue, it might collect the atom out from under this code (mismatch
case, obviously).  You want to set argv[1] = name; after the JS_IdToValue call
succeeds.

Also, you are not respecting prevailing style: don't brace those single-line
return statement then clauses at all (but if you had to brace, put the opening
brace on its own line).

Fix the bug and nit, and sr=[e-mail].

/be

Comment 13

[Johnny Stenback (:jst)]

Attachment: Fix the issues brought up by brendan.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 99619 [details] [diff] [review]
Fix the issues brought up by brendan.

sr=[e-mail]

/be

Comment 15

[David Bradley]

Comment on attachment 99619 [details] [diff] [review]
Fix the issues brought up by brendan.

r=dbradley

Good catch. The only thing that concerns me is that if another such function is
added it may end up with the same problem. Insuring the name is atomized at
XPCCallContext::SetName or within the FindMember functions is probably ideal,
but more risky. In the wrapped JS case, XPConnect asserts that the jsval's
coming in are atomized. It would be nice if we could ensure this within the
FindMember functions. Moving this assert to XPCCallContext might be beneficial.
At least it would have caught this instance.

Comment 16

[jag (Peter Annema)]

Comment on attachment 99619 [details] [diff] [review]
Fix the issues brought up by brendan.

r=jag

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Last night on IRC, Johnny gave me permission to check this in for him if
dbradley was one of the r=s.  He is, so I just landed this.

Thanks all, for getting this bug fixed so quickly!

Comment 18

[Phil Schwartau]

Verified FIXED by comparing these Mozilla trunk binaries on WinNT:

                         2002-09-11-xx
                         2002-09-24-xx 

STEPS TO REPRODUCE
un-jar comm.jar, replace pageInfo.xul with the testcase, and re-jar:

1. cd bin/chrome
2. jar xf comm.jar
3. cd content
4. cd navigator
5. replace pageInfo.xul with the testcase above
6. cd ../..
7. jar cf comm.jar content
8. Launch the browser
9. Do View > Page Info to bring up the testcase
10. Click on the three buttons 


binary 2002-09-11-xx: the try...catch code in the testcase caught
and alerted exceptions generated by clicking on the 2nd and 3rd buttons:

  Exception.. "Component returned failure code: NS_ERROR_XPC_BAD_CONVERT_JS
  in nsIXPCComponents.lookupMethod ..."


binary 2002-09-24-xx:  I got no exceptions by clicking any of the buttons.