Re-disallow embeddings reaching into our guts (jsnum.h)

RESOLVED FIXED in mozilla7

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

Apparently (or so I understand it, at a skim, not investigating thoroughly right now) bug 548205 deliberately removed jsnum.h from INSTALLED_HEADERS.  In order to make a patch compile, I (re?)added it in bug 636079.  Undo that, probably by moving the offending method to an inlines file which doesn't need jsnum.h installed.

More important, however, we should add a comment by INSTALLED_HEADERS explaining the rules for when something should or shouldn't be there.  I didn't know it was bad to have jsnum.h here, but if it is, we should explain when files should and shouldn't be added to that list, to avoid future mistakes.
Your concern is somehow related to bug 554088. We should only install public headers, and there should be a clear distinction between public and private APIs, which (sadly) is far from the case at the moment.

More to the point, the problem is somehow amplified by the fact that we usually rely on being able to include private bits without extra changes in Makefiles, and that relies on headers being copied in dist/include. The fact that all headers from there basically end up in the SDK is a problem.

So I guess a dev-platform discussion should be raised on the subject of private headers vs. public headers and how we want them to be dealt with (considering the implication this would have on what developers would need to do when they add new headers)
Created attachment 538065 [details] [diff] [review]
Patch

I have no idea what the right comment is to explain when things can and can't be added to INSTALLED_HEADERS, so I'm going to hope fear will keep the loca^H^H^H^developers in line.  Fear of this comment.
Attachment #538065 - Flags: review?(jimb)

Comment 3

6 years ago
Comment on attachment 538065 [details] [diff] [review]
Patch

Review of attachment 538065 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/Makefile.in
@@ +190,5 @@
> +##########################################################################
> +# Changes to internal header files, used externally, massively slow down #
> +# browser builds.  Don't add new files here unless you know what you're  #
> +# doing!                                                                 #
> +##########################################################################

I think you should drop the scary border, but the content seems fine.
Attachment #538065 - Flags: review?(jimb) → review+
http://hg.mozilla.org/tracemonkey/rev/9374da0a2b1a
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla7
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/9374da0a2b1a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 554088
You need to log in before you can comment on or make changes to this bug.