The default bug view has changed. See this FAQ.

nsHTMLDocumentSH::CallToGetPropMapper uses JS_GetStringCharsAndLength but does not anchor the result

RESOLVED FIXED in mozilla18



5 years ago
5 years ago


(Reporter: jdm, Assigned: jkitch)


Mac OS X
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [mentor=jdm][lang=c++])


(1 attachment, 3 obsolete attachments)



5 years ago
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

5 years ago
Created attachment 663669 [details] [diff] [review]
Anchor string in nsHTMLDocumentSH::CallToGetPropMapper

It compiles and launches, but are there any tests I should run?
Attachment #663669 - Flags: review?(josh)

Comment 2

5 years ago
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!
Attachment #663669 - Flags: review?(josh)
Attachment #663669 - Flags: review?(jorendorff)
Attachment #663669 - Flags: feedback+
Attachment #663669 - Flags: review?(jorendorff) → review+


5 years ago
Keywords: checkin-needed

Comment 3

5 years ago
Actually this should get a proper r=jorendorff line on the commit message before it's checked in :P
Keywords: checkin-needed

Comment 4

5 years ago
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.
Attachment #663669 - Attachment is obsolete: true

Comment 5

5 years ago
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?
Attachment #665305 - Attachment is obsolete: true

Comment 6

5 years ago
Thanks! You've also now got the ability to do things like set flags; enjoy.
Keywords: checkin-needed
Assignee: nobody → jkitch.bug
Flags: in-testsuite-
Keywords: checkin-needed
And backed out for build bustage. Did this even compile locally?

../../../../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

5 years ago
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.
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

5 years ago
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.
Attachment #665316 - Attachment is obsolete: true

Looks good, thanks :). It's in my landing queue.
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.