Closed Bug 614561 Opened 9 years ago Closed 9 years ago

pprint() does not take string arguments any more

Categories

(DevTools :: General, defect, P1)

x86
Windows XP
defect

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: vladmaniac, Assigned: rcampbell)

References

Details

(Keywords: regression, Whiteboard: [Web-Console-Testday])

Attachments

(1 file)

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
taking this. I touched it last.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
blocking2.0: --- → ?
(In reply to comment #1)
> taking this. I touched it last.

Sounds like this broke via a TM-merge?
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.
Rob, this is reproducible so far until the beta7 release candidates. Those worked.
Attached patch [checked-in] fixSplinter Review
simple fix to catch an exception in our unwrap() method.

with testcase!
Attachment #493087 - Flags: review?(gavin.sharp)
(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.
Blocks: devtools4
Priority: -- → P1
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.
Attachment #493087 - Flags: review?(gavin.sharp)
Attachment #493087 - Flags: review+
Attachment #493087 - Flags: approval2.0+
Comment on attachment 493087 [details] [diff] [review]
[checked-in] fix

http://hg.mozilla.org/mozilla-central/rev/1429940f0fd2
Attachment #493087 - Attachment description: fix → [checked-in] fix
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: [Web-Console-Testday]
blocking2.0: ? → betaN+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.