932102 - Fix the code in nsStyleUtil::AppendAngleValue to not annoy the exact rooting static analysis

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Terrence Cole [:terrence]]

The code in this method puts an nsROCSSPrimitiveValue on the stack. This class contains a CSSValue, which inherits from nsWrapperCache, which puts both a JS::Heap<JSObject*> and a refcounted object on the stack. Neither of these primitives are meant for stack use. The only reason this happens to work currently is that the JSObject* is never initialized and the refcount is never incremented, however, this is ugly and a bit unsafe. Moreover, since the exact rooting static analysis does not do data-flow, it emits a false-positive here. We should change the code to avoid these pitfalls.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems like all it's doing underneath is nsString::AppendFloat.  Perhaps there should be a static helper function somewhere (AppendCSSNumber) that calls into nsString::AppendFloat (which would be used both here, and in the places in nsROCSSPrimtiveValue and elsewhere that currently call AppendFloat directly) in case we want to change it at some point -- but otherwise it seems like this doesn't need to use nsROCSSPrimitiveValue.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug932102-css-append-angle

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8334954 [details] [diff] [review]
bug932102-css-append-angle

Could you:
 (1) make the definition of the method inline in nsStyleUtil.h so there's no performance overhead, and

 (2) make all the AppendFloat calls in nsROCSSPrimitiveValue::GetCssText also use the method?

r=dbaron with that

Comment 4

[Jon Coppeard (:jonco)]

Pushed with review comments applied:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dedb73e93f5

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3dedb73e93f5

Comment 6

[Jonathan Kew [:jfkthame]]

This broke non-unified builds. nsStyleUtil.h only #includes nsStringFwd.h, which is not enough to let you call aResult.AppendFloat(aNumber) in that new inline method.