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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Huseby [:huseby]]

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.

Comment 1

[Dave Huseby [:huseby]]

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

Comment 2

[Dave Huseby [:huseby]]

Attachment: Bug_1116189.patch

This patch uses the cx-less nsAutoJSString::init from Bug 1115375 where it makes sense.

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Dave Huseby [:huseby]]

Attachment: Bug_1116189.patch

I updated the init's to check for failure and i think i understood your other comments re: slat and slon.

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Dave Huseby [:huseby]]

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

[Dave Huseby [:huseby]]

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:

Comment 8

[Dave Huseby [:huseby]]

Now that Bug 1115375 has landed into integration, this one can go in too.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks like this is in the same boat as bug 1114667.

Comment 13

[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 14

[Josh Cheng [:josh]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.