Closed Bug 1116189 Opened 6 years ago Closed 5 years ago

fix up the dom uses of nsAutoJSString::Init to use the cx-less interface where it makes sense

Categories

(Core :: DOM: Device Interfaces, 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)

There are a number of places in the dom code where we handle Observe events that contain wrapped JSObjects that get converted to JS::Values and later nsStrings.  With a better cx-less nsAutoJSString::Init function in bug 1115375 all of that code should be cleaned up to use the new function.  This limits the use of the default/safe JS context in the Observe handlers to just rooting purposes and the nsAutoJSString can handle finding the correct context/compartment for us.
Blocks: Privacy
so I'm testing the patches for this and Bug 1115375.  no more freezes but the privacy panel itself appears to be buggy.
Attached patch Bug_1116189.patch (obsolete) — Splinter Review
This patch uses the cx-less nsAutoJSString::init from Bug 1115375 where it makes sense.
Attachment #8562548 - Flags: review?(bobbyholley)
Comment on attachment 8562548 [details] [diff] [review]
Bug_1116189.patch

Review of attachment 8562548 [details] [diff] [review]:
-----------------------------------------------------------------

You need to null-check the return value of init and handle OOM.

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +434,5 @@
>      return;
>    }
>  
>    nsresult rv;
> +  nsString slat(Substring(str, 1, comma -1));

Hm?

@@ +439,4 @@
>    nsString slon(Substring(str, comma + 1));
>    double lat = slat.ToDouble(&rv);
> +  GPSLOG("split strings: |%s| |%s|", NS_ConvertUTF16toUTF8(slat).get(),
> +         NS_ConvertUTF16toUTF8(slon).get());

This too.
Attachment #8562548 - Flags: review?(bobbyholley) → review-
I updated the init's to check for failure and i think i understood your other comments re: slat and slon.
Attachment #8562548 - Attachment is obsolete: true
Attachment #8564429 - Flags: review?(bobbyholley)
Comment on attachment 8564429 [details] [diff] [review]
Bug_1116189.patch

Review of attachment 8564429 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that removed.

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +447,2 @@
>    nsresult rv;
> +  nsString slat(Substring(str, 1, comma - 1));

I still don't understand why you're changing this line and subtracting 1. That seems like a behavior change unrelated to this patch which I shouldn't be the one to review.
Attachment #8564429 - Flags: review?(bobbyholley) → review+
blocking-b2g: --- → 2.2+
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #5)
> Comment on attachment 8564429 [details] [diff] [review]
> Bug_1116189.patch
> 
> Review of attachment 8564429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that removed.
> 
> ::: dom/geolocation/nsGeolocationSettings.cpp
> @@ +447,2 @@
> >    nsresult rv;
> > +  nsString slat(Substring(str, 1, comma - 1));
> 
> I still don't understand why you're changing this line and subtracting 1.
> That seems like a behavior change unrelated to this patch which I shouldn't
> be the one to review.

It was just an off-by-one error I found while testing.  I don't think it requires review.
Comment on attachment 8564429 [details] [diff] [review]
Bug_1116189.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 #8564429 - Flags: approval-mozilla-b2g37?
Attachment #8564429 - Flags: approval-mozilla-b2g34?
Now that Bug 1115375 has landed into integration, this one can go in too.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a30cdc2cc8d8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8564429 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8564429 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
blocking-b2g: 2.2+ → 2.1+
Looks like this is in the same boat as bug 1114667.
Flags: needinfo?(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.
Flags: needinfo?(huseby) → needinfo?(jocheng)
Hi Dave,
Thanks for the info, I just removed the flags on 2.1.
Flags: needinfo?(jocheng)
Attachment #8564429 - Flags: approval-mozilla-b2g34+
Per the last couple comments in the bug, this should be set back to 2.2+, but I don't have the ability to do so myself.
blocking-b2g: 2.1+ → 2.2?
Flags: needinfo?(bbajaj)
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(bbajaj)
You need to log in before you can comment on or make changes to this bug.