Closed Bug 1115375 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
blocking-b2g 2.2+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1116189
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().
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Flags: needinfo?(huseby)
Blocks: Privacy
so I'm testing the patches for this and Bug 1116189.  no more freezes but the privacy panel itself appears to be buggy.
Flags: needinfo?(huseby)
Attached patch Bug_1115375.patch (obsolete) — Splinter Review
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.
Attachment #8562547 - Flags: review?(bobbyholley)
Attachment #8562547 - Flags: review?(bobbyholley) → review+
Keywords: checkin-needed
Please try to avoid including nsContentUtils.h in headers.
Holding off on checkin until comment 5 is resolved.
Keywords: checkin-needed
(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.
Flags: needinfo?(Ms2ger)
blocking-b2g: --- → 2.2+
Any suggestion for not including nsContentUtils.h?
(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().
Duh.  Just moved the implementation to nsJSUtils.cpp where the nsContentUtils.h was already included.
Attachment #8562547 - Attachment is obsolete: true
Attachment #8566798 - Flags: review?(bobbyholley)
No longer includes nsContentUtils.h
Flags: needinfo?(Ms2ger)
Attachment #8566798 - Flags: review?(bobbyholley) → review+
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:
Attachment #8566798 - Flags: approval-mozilla-b2g37?
Attachment #8566798 - Flags: approval-mozilla-b2g34?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/398df560c211
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8566798 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8566798 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
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.
Flags: needinfo?(huseby) → needinfo?(jocheng)
Hi Dave,
Thanks for the info, I just removed the flags on 2.1.
Flags: needinfo?(jocheng)
Attachment #8566798 - Flags: approval-mozilla-b2g34+
Depends on: 1220682
You need to log in before you can comment on or make changes to this bug.