Last Comment Bug 662231 - Re-disallow embeddings reaching into our guts (jsnum.h)
: Re-disallow embeddings reaching into our guts (jsnum.h)
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 636079
Blocks: 548205 554088
  Show dependency treegraph
 
Reported: 2011-06-06 00:45 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-01-07 07:24 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.09 KB, patch)
2011-06-08 11:08 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jimb: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-06 00:45:15 PDT
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.
Comment 1 Mike Hommey [:glandium] 2011-06-06 01:01:49 PDT
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)
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-08 11:08:20 PDT
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.
Comment 3 Jim Blandy :jimb 2011-06-08 11:13:14 PDT
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.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-13 18:15:51 PDT
http://hg.mozilla.org/tracemonkey/rev/9374da0a2b1a
Comment 5 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:12:35 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/9374da0a2b1a

Note You need to log in before you can comment on or make changes to this bug.