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

unchecked call to ToJSValue without checking the result

RESOLVED FIXED in Firefox 36

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

Trunk
Firefox 36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35+ wontfix, firefox36+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
nsDocShell::PopProfileTimelineMarkers calls ToJSValue without
checking the result.  This is a no-no.
See bug 1095728.
(Assignee)

Updated

3 years ago
Assignee: nobody → ttromey
Blocks: 1095728
[Tracking Requested - why for this release]:
Blocks: 1050376
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
(Assignee)

Comment 2

3 years ago
Created attachment 8519957 [details] [diff] [review]
check ToJSValue result in PopProfileTimelineMarkers

I could not think of a reasonable way to write a test for this, so
tested just by rebuilding.
(Assignee)

Updated

3 years ago
Attachment #8519957 - Flags: review?(bugs)

Comment 3

3 years ago
Comment on attachment 8519957 [details] [diff] [review]
check ToJSValue result in PopProfileTimelineMarkers

Always {} with 'if', and I would return an error value if ToJSValue fails.
But it is not too clear whether ClearProfileTimelineMarkers() and 
mProfileTimelineMarkers.SwapElements(keptMarkers); should be called in that case.
Probably yes.
Attachment #8519957 - Flags: review?(bugs) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8520754 [details] [diff] [review]
check ToJSValue result in PopProfileTimelineMarkers

Here's an updated version that returns an error and still arranges to
clear the markers, etc.
Attachment #8519957 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8520754 - Flags: review?(bugs)

Comment 5

3 years ago
Comment on attachment 8520754 [details] [diff] [review]
check ToJSValue result in PopProfileTimelineMarkers

And still keep the JS_ClearException thing inside the if.
I think that would give the sanest behavior, given that this xpidl method.
Attachment #8520754 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8520807 [details] [diff] [review]
check ToJSValue result in PopProfileTimelineMarkers

Third time's the charm.  Carrying over the r+ & marking for checkin.
(Assignee)

Updated

3 years ago
Attachment #8520754 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox35: ? → +
tracking-firefox36: ? → +
Hi, could you provide a try link ? Thanks!
Flags: needinfo?(ttromey)
Keywords: checkin-needed
(Assignee)

Comment 8

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7019f5bb6fa4
(Assignee)

Comment 9

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #7)
> Hi, could you provide a try link ? Thanks!

Sorry about that.  I will learn, I promise!
I've started it now.
Flags: needinfo?(ttromey)
(Assignee)

Comment 10

3 years ago
It's done now and the results look reasonable to me.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf5a7209f06
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2bf5a7209f06
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox36: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Please nominate for beta uplift approval consideration if low risk.  We'd need to take this in Monday Dec 22 beta otherwise it will have to wait and ship with FF36.
Flags: needinfo?(ttromey)
(Assignee)

Comment 14

3 years ago
I think this probably isn't important enough to nominate.
Also I think the ToJSValue issue can probably only occur on out-of-memory,
so neither the original code nor the patched code paths are readily
testable.
Flags: needinfo?(ttromey)
status-firefox35: affected → wontfix
Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl
Component: Developer Tools: Timeline → Developer Tools: Performance Tools (Profiler/Timeline)
You need to log in before you can comment on or make changes to this bug.