Last Comment Bug 718293 - nsHTMLDocumentSH::CallToGetPropMapper uses JS_GetStringCharsAndLength but does not anchor the result
: nsHTMLDocumentSH::CallToGetPropMapper uses JS_GetStringCharsAndLength but doe...
Status: RESOLVED FIXED
[mentor=jdm][lang=c++]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: James Kitchener (:jkitch)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-15 08:03 PST by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-09-29 21:10 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Anchor string in nsHTMLDocumentSH::CallToGetPropMapper (788 bytes, patch)
2012-09-22 04:09 PDT, James Kitchener (:jkitch)
jorendorff: review+
josh: feedback+
Details | Diff | Splinter Review
update commit message (r=jorendorff) (809 bytes, patch)
2012-09-27 02:29 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
update commit message (r=jorendorff) (784 bytes, patch)
2012-09-27 02:38 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
add anchor (compiles) (784 bytes, patch)
2012-09-28 04:47 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-01-15 08:03:05 PST
Obtaining a jschar* result direct can be dangerous, because the JS engine's stack scanner does not know the string is still in use. The result should be stored in a JS::Anchor to ensure safety.
Comment 1 James Kitchener (:jkitch) 2012-09-22 04:09:26 PDT
Created attachment 663669 [details] [diff] [review]
Anchor string in nsHTMLDocumentSH::CallToGetPropMapper

It compiles and launches, but are there any tests I should run?
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-22 06:20:14 PDT
Comment on attachment 663669 [details] [diff] [review]
Anchor string in nsHTMLDocumentSH::CallToGetPropMapper

Review of attachment 663669 [details] [diff] [review]:
-----------------------------------------------------------------

No, this is something that requires great luck to be able to test. Thanks for doing this!
Comment 3 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-26 11:49:28 PDT
Actually this should get a proper r=jorendorff line on the commit message before it's checked in :P
Comment 4 James Kitchener (:jkitch) 2012-09-27 02:29:58 PDT
Created attachment 665305 [details] [diff] [review]
update commit message (r=jorendorff)

Update the commit message, no other changes.

Could you please set the checkin-needed keyword again?  I am unable to do so.
Comment 5 James Kitchener (:jkitch) 2012-09-27 02:38:40 PDT
Created attachment 665316 [details] [diff] [review]
update commit message (r=jorendorff)

This one has unix-style newlines.  

Am I correct in assuming that it matters?
Comment 6 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-27 07:49:59 PDT
Thanks! You've also now got the ability to do things like set flags; enjoy.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-09-27 20:21:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/93b39d0f7d17
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-27 20:26:44 PDT
And backed out for build bustage. Did this even compile locally?
https://hg.mozilla.org/integration/mozilla-inbound/rev/b30bd7055fcb

https://tbpl.mozilla.org/php/getParsedLog.php?id=15611540&tree=Mozilla-Inbound

../../../../dom/base/nsDOMClassInfo.cpp:9176:37: error: expected ';' at end of declaration
  JS::Anchor<JSString *> anchor(str)
                                    ^
                                    ;
1 error generated.
make[7]: *** [nsDOMClassInfo.o] Error 1
Comment 9 James Kitchener (:jkitch) 2012-09-27 22:25:45 PDT
I sincerely apologise for this.

What I submitted was an earlier iteration.  I had neglected to refresh the patch after correcting that bug.

I'll upload a updated patch when I can verify that it builds locally.   Once I have done so, are there any additional procedures/reviews that need to be performed?

Once again, my apologies.  It won't happen again.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-09-28 03:41:56 PDT
I'd like to see a green Try run before pushing it again. I can push it if nobody else beats me to it.
Comment 11 James Kitchener (:jkitch) 2012-09-28 04:47:55 PDT
Created attachment 665861 [details] [diff] [review]
add anchor (compiles)

This one is definitely up to date and compiles locally.

Would you mind doing the try run?  I don't think I have the required permissions.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-09-29 07:48:45 PDT
https://tbpl.mozilla.org/?tree=Try&rev=41211fe04f8d

Looks good, thanks :). It's in my landing queue.
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-09-29 09:39:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecedd932c25b
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-29 21:10:31 PDT
https://hg.mozilla.org/mozilla-central/rev/ecedd932c25b

Note You need to log in before you can comment on or make changes to this bug.