Closed Bug 1096329 Opened 10 years ago Closed 10 years ago

unchecked call to ToJSValue without checking the result

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Product:
DevTools
Component:
Performance Tools (Profiler/Timeline)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox35+ wontfix, firefox36+ fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Tracking Flags:
Tracking Status
firefox35 + wontfix
firefox36 + fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(1 file, 2 obsolete files)

check ToJSValue result in PopProfileTimelineMarkers
(deleted), patch
Details | Diff | Splinter Review
nsDocShell::PopProfileTimelineMarkers calls ToJSValue without checking the result. This is a no-no. See bug 1095728.
Assignee: nobody → ttromey
Blocks: 1095728
[Tracking Requested - why for this release]:
Blocks: 1050376
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
I could not think of a reasonable way to write a test for this, so tested just by rebuilding.
Attachment #8519957 - Flags: review?(bugs)
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-
Here's an updated version that returns an error and still arranges to clear the markers, etc.
Attachment #8519957 - Attachment is obsolete: true
Attachment #8520754 - Flags: review?(bugs)
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+
Third time's the charm. Carrying over the r+ & marking for checkin.
Attachment #8520754 - Attachment is obsolete: true
Keywords: checkin-needed
Hi, could you provide a try link ? Thanks!
Flags: needinfo?(ttromey)
Keywords: checkin-needed
(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)
It's done now and the results look reasonable to me.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2bf5a7209f06
Status: NEW → RESOLVED
Closed: 10 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)
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)
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)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: