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

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: huseby, Assigned: huseby)

Tracking

unspecified
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

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

Updated

5 years ago
Blocks: 1116189
Assignee

Comment 1

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

Updated

5 years ago
Flags: needinfo?(huseby)

Updated

5 years ago
Blocks: Privacy
Assignee

Comment 3

4 years ago
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)
Assignee

Comment 4

4 years ago
Posted 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+
Assignee

Updated

4 years ago
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)

Updated

4 years ago
blocking-b2g: --- → 2.2+
Assignee

Comment 8

4 years ago
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().
Assignee

Comment 10

4 years ago
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)
Assignee

Comment 11

4 years ago
No longer includes nsContentUtils.h
Flags: needinfo?(Ms2ger)
Attachment #8566798 - Flags: review?(bobbyholley) → review+
Assignee

Comment 12

4 years ago
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?
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/398df560c211
Status: ASSIGNED → RESOLVED
Closed: 4 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+
Assignee

Comment 18

4 years ago
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+
You need to log in before you can comment on or make changes to this bug.