614561 - pprint() does not take string arguments any more

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[Maniac Vlad Florin (:vladmaniac)]

Build ID:

Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101123 Firefox/4.0b8pre

Steps:
1. Open console
2. Type pprint("string"), and string can be any char you prefer to test with 

Behavior: 

Console echoes exception
[Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/HUDService.jsm :: unwrap :: line 710"  data: no]

Expected: 
Result as follows: 
"s"
"t"
"r"
"i"
"n"
"g"

in console window

Comment 1

[Rob Campbell [:rc] (:robcee)]

taking this. I touched it last.

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> taking this. I touched it last.

Sounds like this broke via a TM-merge?

Comment 3

[Rob Campbell [:rc] (:robcee)]

hm, I hadn't thought of that. I thought it might just be our unwrap function failing trying to unwrap a string literal. Not sure we were testing that before.

vlad, can you retest this on a nightly from a few days ago? (e.g., Nov. 20) Let us know if it's still a problem and if it isn't, we'll have to point this at the JS team and wrapper gurus.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Rob, this is reproducible so far until the beta7 release candidates. Those worked.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Attachment: [checked-in] fix

simple fix to catch an exception in our unwrap() method.

with testcase!

Comment 6

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #4)
> Rob, this is reproducible so far until the beta7 release candidates. Those
> worked.

that's what I thought. Thanks for the retest.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 493087 [details] [diff] [review]
[checked-in] fix

I guess this is OK, but It seems like it would be nicer to avoid calling unwrap() when it wouldn't be necessary (i.e. when we have a primitive value). nit: use try {return XPCNW.unwrap(aObj) } catch (ex) {return aObj;} to avoid the need for the unwrappedObject temp variable.

We could relatively easily make XPCNativeWrapper.unwrap() not throw in that case (by reworking the JSVAL_IS_PRIMITIVE check in http://hg.mozilla.org/mozilla-central/annotate/60d9203fdc35/js/src/xpconnect/src/XPCWrapper.cpp#l59 ), but I don't know whether that has other implications. Maybe worth investigating in a followup.

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 493087 [details] [diff] [review]
[checked-in] fix

http://hg.mozilla.org/mozilla-central/rev/1429940f0fd2