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

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Josh Matthews [:jdm]]

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)]

Attachment: Anchor string in nsHTMLDocumentSH::CallToGetPropMapper

It compiles and launches, but are there any tests I should run?

Comment 2

[Josh Matthews [:jdm]]

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]]

Actually this should get a proper r=jorendorff line on the commit message before it's checked in :P

Comment 4

[James Kitchener (:jkitch)]

Attachment: 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)]

Attachment: update commit message (r=jorendorff)

This one has unix-style newlines.  

Am I correct in assuming that it matters?

Comment 6

[Josh Matthews [:jdm]]

Thanks! You've also now got the ability to do things like set flags; enjoy.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93b39d0f7d17

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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)]

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]]

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)]

Attachment: 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]]

https://tbpl.mozilla.org/?tree=Try&rev=41211fe04f8d

Looks good, thanks :). It's in my landing queue.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecedd932c25b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ecedd932c25b