1096329 - unchecked call to ToJSValue without checking the result

Status: RESOLVED FIXED

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

Description

[Tom Tromey :tromey]

nsDocShell::PopProfileTimelineMarkers calls ToJSValue without
checking the result.  This is a no-no.
See bug 1095728.

Comment 1

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

Comment 2

[Tom Tromey :tromey]

Attachment: check ToJSValue result in PopProfileTimelineMarkers

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Tom Tromey :tromey]

Attachment: check ToJSValue result in PopProfileTimelineMarkers

Here's an updated version that returns an error and still arranges to
clear the markers, etc.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Tom Tromey :tromey]

Attachment: check ToJSValue result in PopProfileTimelineMarkers

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

Comment 7

[Carsten Book [:Tomcat]]

Hi, could you provide a try link ? Thanks!

Comment 8

[Tom Tromey :tromey]

https://tbpl.mozilla.org/?tree=Try&rev=7019f5bb6fa4

Comment 9

[Tom Tromey :tromey]

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

Comment 10

[Tom Tromey :tromey]

It's done now and the results look reasonable to me.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf5a7209f06

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/2bf5a7209f06

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 14

[Tom Tromey :tromey]

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.

Comment 15

[David Lawrence [:dkl]]

Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl