If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: huseby, Assigned: huseby)

Tracking

(Blocks: 1 bug)

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

3 years ago
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.

Updated

3 years ago
Blocks: 1115356

Updated

3 years ago
Blocks: 1057675
(Assignee)

Comment 1

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

Comment 2

3 years ago
Created attachment 8562548 [details] [diff] [review]
Bug_1116189.patch

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

Comment 4

3 years ago
Created attachment 8564429 [details] [diff] [review]
Bug_1116189.patch

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+

Updated

3 years ago
blocking-b2g: --- → 2.2+
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
(Assignee)

Comment 6

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

Comment 7

3 years ago
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?
(Assignee)

Comment 8

3 years ago
Now that Bug 1115375 has landed into integration, this one can go in too.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a30cdc2cc8d8
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a30cdc2cc8d8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

3 years ago
Attachment #8564429 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b5a532c7f606
status-b2g-v2.1: --- → affected
status-b2g-v2.2: affected → fixed
status-b2g-master: affected → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix

Updated

3 years ago
Attachment #8564429 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+

Updated

3 years ago
blocking-b2g: 2.2+ → 2.1+
Looks like this is in the same boat as bug 1114667.
Flags: needinfo?(huseby)
(Assignee)

Comment 13

3 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)

Comment 14

3 years ago
Hi Dave,
Thanks for the info, I just removed the flags on 2.1.
status-b2g-v2.1: affected → ---
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)

Updated

3 years ago
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.