1302826 - stylo: The forward declaration of Servo_NodeData_Drop in nsINode.h is unsound.

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

We're declaring it with a different prototype than in ServoBindings.h, which is at least dubious, if not illegal, for an extern "C" function.

This was caught during the bindgen upgrade I'm trying to land.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1302826: Rename ServoNodeData's default destructor to Servo_NodeData_DefaultDelete.

The previous version of this was unsound, and was declaring the same extern "C"
function with two different prototypes.

Move the implementation to ServoBindings.cpp, with a doc comment that is also
generated in the Servo file.

Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/78814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78814/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1302826: Wrap the extern "C" implemented functions with a proper extern "C" block.

Not sure if it's totally required, I guess not, but I think it's worth it, since
the compiler will error in conflicting declarations now.

Review commit: https://reviewboard.mozilla.org/r/78812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78812/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Why do you think it is unsound? As far as the two declarations can appear in one translation unit, the compiler will raise an error if their type doesn't match.

Effectively, the two declarations do have same prototype to C++ compiler, because ServoNodeDataOwned is just an alias of ServoNodeData*. Adding another layer of indirection would not add any soundness since nothing stops you from mixing up ServoNodeDataOwned and ServoNodeData*.

If the difference between prototypes confuses bindgen, you should just copy the typedef of ServoNodeDataOwned to nsINode (which is allowed in C++ as far as typedef is effectively identical), and update the declaration there, rather than adding another function which doesn't add any soundness, but adds an unnecessary function call in runtime.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8791396 [details]
Bug 1302826: Rename ServoNodeData's default destructor to Servo_NodeData_DefaultDelete.

https://reviewboard.mozilla.org/r/78814/#review77460

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #3)
> Why do you think it is unsound? As far as the two declarations can appear in
> one translation unit, the compiler will raise an error if their type doesn't
> match.
> 
> Effectively, the two declarations do have same prototype to C++ compiler,
> because ServoNodeDataOwned is just an alias of ServoNodeData*. Adding
> another layer of indirection would not add any soundness since nothing stops
> you from mixing up ServoNodeDataOwned and ServoNodeData*.
> 
> If the difference between prototypes confuses bindgen, you should just copy
> the typedef of ServoNodeDataOwned to nsINode (which is allowed in C++ as far
> as typedef is effectively identical), and update the declaration there,
> rather than adding another function which doesn't add any soundness, but
> adds an unnecessary function call in runtime.

Pffft, somehow I thought ServoNodeDataOwned was a new type after the refactor, gah. I'm sorry.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I'm not sure whether we really want the second patch. The only thing we get from that patch is that we can catch undesired function overload, but I don't think that is something we generally use any technique to stop. What we lose is writing template functions near their callsites, which could be more painful I suppose.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8791397 [details]
Bug 1302826: Wrap the extern "C" implemented functions with a proper extern "C" block.

https://reviewboard.mozilla.org/r/78812/#review77462