1115375 - [JSAPI] add nsAutoJSString::Init that doesn't require a JSContext

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Dave Huseby [:huseby]]

I talked to :bholley about the right way to get and nsString from a JS::Value that is a string when we don't already have a JS context.  The solution is to add an nsAutoJSString::Init that builds the context internally.

Comment 1

[Dave Huseby [:huseby]]

Bobby,

You and I worked out a way to add an nsAutoJSString::Init that took just a JS::Value parameter without a JS::Context.  I took some notes, but they are incomplete and my brain has dumped the other details.  I remember you saying that dealing with strings is slightly different.  What's the proper way to get a JS context from a JS::Value that you think is a string, but could be anything.  Would it be enough to just get the RootingCxForThread?

I wanted to fix up functions like GeolocationSetting::HandleFixedCoordsChange (http://is.gd/fInfmD) which takes a JS::Value it expects to be a string and converts it to an nsString for processing:

> void
> GeolocationSetting::HandleFixedCoordsChange(const JS::Value& aVal)
> {
>   AutoJSAPI jsapi;
>   jsapi.Init();
>   JSContext* cx = jsapi.cx();
>   nsString str;
>   if (!aVal.isString() || !AssignJSString(cx, str, aVal.toString()) || str.IsEmpty()) {
>     return;
>   }
>   //....
> }

What I'd like to do instead is something like the following and have the string rooted and referenced properly by the nsAutoJSString class:

> void
> GeolocationSetting::HandleFixedCoordsChange(const JS::Value& aVal)
> {
>   if (!aVal.isString())
>     return;
>   nsAutoJSString str(aVal);
>   //....
> }

Can you help me out?  I just need to know the what the proper way to root a JS::Value is and what makes JSStrings different than regular JS objects.  I can write the rest of the ::Init().

Comment 2

[Bobby Holley (:bholley)]

(In reply to Dave Huseby [:huseby] from comment #1)
> You and I worked out a way to add an nsAutoJSString::Init that took just a
> JS::Value parameter without a JS::Context.  I took some notes, but they are
> incomplete and my brain has dumped the other details.  I remember you saying
> that dealing with strings is slightly different.  What's the proper way to
> get a JS context from a JS::Value that you think is a string, but could be
> anything.  Would it be enough to just get the RootingCxForThread?

Yes.

Comment 3

[Dave Huseby [:huseby]]

so I'm testing the patches for this and Bug 1116189.  no more freezes but the privacy panel itself appears to be buggy.

Comment 4

[Dave Huseby [:huseby]]

Attachment: Bug_1115375.patch

I think this is all that needs to happen to make a cx-less nsAutoJSString.  The only thing I can think of that might be wrong is the RootingCxForThread gets passed to the other ::init functions.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please try to avoid including nsContentUtils.h in headers.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Holding off on checkin until comment 5 is resolved.

Comment 7

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #5)
> Please try to avoid including nsContentUtils.h in headers.

Do you have an alternative suggestion? Note that with unified builds, build times are much less affected by these things now.

Comment 8

[Dave Huseby [:huseby]]

Any suggestion for not including nsContentUtils.h?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Dave Huseby [:huseby] from comment #8)
> Any suggestion for not including nsContentUtils.h?

I guess you could define a non-inline helper in nsJSUtils.h whose implementation in nsJSUtils.cpp invokes nsContentUtils::RootingCxForThread().

Comment 10

[Dave Huseby [:huseby]]

Attachment: Bug_1115375.patch

Duh.  Just moved the implementation to nsJSUtils.cpp where the nsContentUtils.h was already included.

Comment 11

[Dave Huseby [:huseby]]

No longer includes nsContentUtils.h

Comment 12

[Dave Huseby [:huseby]]

Comment on attachment 8566798 [details] [diff] [review]
Bug_1115375.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1057675
User impact if declined: phone freezes and restarts when using geolocation.
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/398df560c211

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/398df560c211

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ad7ef54f1d86

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/97e2ac4449a5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0390c73a827b

https://treeherder.mozilla.org/logviewer.html#?job_id=89134&repo=mozilla-b2g34_v2_1

Comment 18

[Dave Huseby [:huseby]]

AFAIK we're not shipping the privacy panel in v2.1 or v2.1s.  None of my patches have ever been landed in those branches.  If I'm right, please clear out the "affected" flags or confirm I'm right and I'll do it.

Comment 19

[Josh Cheng [:josh]]

Hi Dave,
Thanks for the info, I just removed the flags on 2.1.