Remove jscntxt.h from INSTALLED_HEADERS

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Depends on: 1 bug)

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(28 attachments, 5 obsolete attachments)

3.68 KB, patch
Igor Bukanov
: review+
Details | Diff | Splinter Review
8.15 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.58 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
8.98 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.80 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.40 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.63 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
4.41 KB, patch
Igor Bukanov
: review+
Details | Diff | Splinter Review
3.60 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.88 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
4.34 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.23 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.30 KB, patch
gal
: review+
Details | Diff | Splinter Review
4.60 KB, patch
gal
: review+
Details | Diff | Splinter Review
3.37 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.22 KB, patch
gal
: review+
Details | Diff | Splinter Review
3.95 KB, patch
luke
: review+
Details | Diff | Splinter Review
12.55 KB, patch
billm
: review+
Details | Diff | Splinter Review
2.92 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.17 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
3.50 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.68 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.23 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.19 KB, patch
mjrosenb
: review+
Details | Diff | Splinter Review
12.72 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
2.76 KB, patch
njn
: review+
Details | Diff | Splinter Review
1.28 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review
2.43 KB, patch
sfink
: review+
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
See bug 672893 comment 13 and following.

Besides bug 332648, content/xul/document/src/nsXULContentSink.cpp and content/base/src/nsScriptLoader.cpp need js::VersionFlags::HAS_XML or VersionHasXML/VersionSetXML.

:jorendorff, :luke, and :billm are ready to take a 1/6 chance of having to fix this bug in order to get it fixed; three more heroes are required.
I volunteer.  Actually, I auto-volunteer for bugs removing any of the installed headers except js*api.h
Volunteer.

Comment 3

6 years ago
Volunteer. Non-API includes must die.
"Give me the victim, give me the victim!"

http://www.youtube.com/watch?v=Qxys99Wn_NE#t=1m23s

I am happy to volunteer, with the caveat that I think I need to finish bug 586842 before doing this, should I be the lucky victim.
(Assignee)

Comment 5

6 years ago
$ python -c 'import random; print random.choice("jorendorff, luke, billm, bhackett, cdleary, gal, Waldo".split(", "))'
Waldo

(I'll be removing one include in bug 677101, fwiw.)
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Depends on: 586842
Well then!  I guess that caveat will actually matter.  (Unless we were waiting for more people to volunteer?  It wasn't clear to me that everyone had a chance to see/volunteer, not in the space of eight hours on a Sunday morning.)

I will probably be busy with that for most, if not all, of the time until August 18, when I disappear for a couple weeks of vacation.  So it's unlikely this bug will see much progress before September.
Duplicate of this bug: 679975
Blocks: 684344
Created attachment 562206 [details] [diff] [review]
patch

I had an odd urge to fix this last night, so here it is.

I definitely cheated here. Although jscntxt.h is no longer part of INSTALLED_HEADERS, I whitelisted several directories by added js/src to LOCAL_INCLUDES in their Makefiles. The directories are XPConnect, jsd, and dom/base. The reason for the last one is that nsGlobalWindow.cpp defines a wrapper, and I couldn't think of a good way to disentangle the wrapper code from jscntxt.h.

Nevertheless, I think the patch is valuable. It should shorten build times and it gets us closer to the goal of a real API. The main problems that we still have to tackle are wrappers and typed arrays. I'm worried that both of these will be difficult to fix.
Assignee: jwalden+bmo → wmccloskey
Attachment #562206 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

6 years ago
Comment on attachment 562206 [details] [diff] [review]
patch

>--- a/dom/workers/ListenerManager.cpp
>+++ b/dom/workers/ListenerManager.cpp

>+  nsTArray<jsval> listeners;
>-    if ((eventIsTrusted || listener->mWantsUntrusted) &&
>-        !listeners.append(listener->mListenerVal)) {
>+    if (eventIsTrusted || listener->mWantsUntrusted) {
>       return false;
>     }
>+    listeners.AppendElement(listener->mListenerVal);

If you use nsTArray infallibly, can you please make it an InfallibleTArray for clarity?

>   }
> 
>-  if (listeners.empty()) {
>+  if (!listeners.Length()) {

IsEmpty()

>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>-		jscntxt.h \
>-		jscrashreport.h \
>-		jsdate.h \
>-		jsemit.h \
>-		jsfun.h \
>-		jsgc.h \
>-		jsgcchunk.h \
>-		jsgcstats.h \
>-		jsinterp.h \
>-		jsiter.h \
>-		jsparse.h \
>-		jsproxy.h \
>-		jsreflect.h \
>-		jsscan.h \
>-		jsscope.h \
>-		jstracer.h \
>-		jswrapper.h \

\o/

>-/* FIXME(bug 332648): Move this into a public header. */

Don't forget to close that bug.

>--- a/js/src/jsdbgapi.h
>+++ b/js/src/jsdbgapi.h
>+#include "jsprvtd.h"
> #include "jsopcode.h"
>-#include "jsprvtd.h"

Why this change?

>--- a/js/src/jsfriendapi.cpp
>+++ b/js/src/jsfriendapi.cpp
>+JS_FRIEND_API(JSVersion)
>+JS_VersionSetXML(JSVersion version, JSBool enable)
>+{
>+    js::VersionSetXML(&version, enable);

Any reason you didn't match the internal signature?


I wonder if nsXPConnect should go into a public header... That seems like a better approach than duplicating those static functions in mozilla::xpconnect. (In any case, Gecko should use those as xpconnect::Foo(), with a 'using namespace mozilla;' at the top of .cpps that aren't yet using namespaces.)

I'm not entirely sure I like all of this patch, but I do think there are a lot of improvements in it. I'll leave it to your reviewer to decide whether it's better to add the LOCAL_INCLUDES or to keep the file in INSTALLED_HEADERS for now, while we take the rest of the patch.

It's rather big, though, and I think the js_GetErrorMessage->JS_GetErrorMessage should definitely go in a separate patch, and perhaps AutoGCRooter and friends as well (if only for ease of review).
Comment on attachment 562206 [details] [diff] [review]
patch

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

So I mentioned the INSTALLED_HEADERS->LOCAL_INCLUDES thing to bhackett, and he thought it smelled a bit dodgy, too, even as a halfway step (especially since that opens Pandora's box for all the internal headers, not just for a small subset of them).  He also said you'd agreed to defer this to later, when dependencies can be cut more thoroughly, which makes sense to me.  So I'm going to cancel this review for now, and hopefully next time around we won't need this one-step-forward-one-and-a-half-steps-back approach.
Attachment #562206 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 11

6 years ago
Shouldn't we land the other parts of the patch, though? Even if we can't remove jscntxt.h from INSTALLED_HEADERS just yet, moving in that direction would be useful already.
I would probably be in favor of various other parts of the patch, but given all the parts are intermingled, it's hard to review and be sure which parts are needed for which changes.  I'm happy to review smaller, self-contained parts in new patches.
Created attachment 564998 [details] [diff] [review]
autorooters patch

I'm updating the this to completely remove jscntxt.h. This first patch just fixes up the autorooters. I put most of the autorooters in jsapi.h. The ones that depend on vectors I put in jsfriendapi.h. I also changed the autorooter implementation to use virtual function dispatch rather than the tagging scheme; I think this is cleaner.
Attachment #562206 - Attachment is obsolete: true
Attachment #564998 - Flags: review?(jwalden+bmo)
Created attachment 565003 [details] [diff] [review]
main patch

This patch mostly just adds a bunch of shadow declarations and accessors to jsfriendapi.h. This parallel's Brian's approach. There are a few other things here, but they're pretty local.

The one thing that might need explanation in the GC trigger stuff from nsJSEnvironment. Previously that code had been reading out of cx->rt->gcTriggerCompartment to figure out if we were doing a compartment GC. This was wrong, and it always got NULL (so it though every GC was a full GC). I fixed this by adding a new flag that is set correctly by the GC.
Attachment #565003 - Flags: review?(jwalden+bmo)

Updated

6 years ago
Duplicate of this bug: 663598
Comment on attachment 564998 [details] [diff] [review]
autorooters patch

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

::: js/src/jsapi.h
@@ +4840,1 @@
>  JS_END_EXTERN_C

Let's move this above this entire section, then we can avoid having oddnesses like this:

#ifdef __cplusplus
JS_END_EXTERN_C

...

JS_BEGIN_EXTERN_C
#endif /* __cplusplus */

/* yes, really there's *nothing in this space */

JS_END_EXTERN_C

@@ +4859,5 @@
> +        JS_ASSERT(this != reinterpret_cast<shadow::Context *>(cx)->autoGCRooters);
> +        reinterpret_cast<shadow::Context *>(cx)->autoGCRooters = this;
> +    }
> +
> +    ~AutoGCRooter() {

Is this not being virtual going to create Windows compiler warnings, given the virtual trace method below?  Or does the = 0 save you?

@@ +4967,5 @@
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    void changeLength(size_t newLength) {
> +        length = ptrdiff_t(newLength);

This cast is no longer necessary, right?

@@ +5011,5 @@
> +};
> +
> +class AutoIdArray : private AutoGCRooter {
> +  public:
> +    AutoIdArray(JSContext *cx, JSIdArray *ida JS_GUARD_OBJECT_NOTIFIER_PARAM)

Newline and appropriate indentation after *ida.  There's no good way to express what the macro does, but at least getting it away from the arguments we expect callers to provide will hurt readability less.

::: js/src/jsfriendapi.h
@@ +300,5 @@
>  
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(Value *vec, size_t len)
> +{
> +    PodZero(vec, len);

Add an assert that |vec <= vec + len|.

@@ +306,5 @@
> +
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(Value *beg, Value *end)
> +{
> +    PodZero(beg, end - beg);

Add an assert that |beg <= end|.

@@ +312,5 @@
> +
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(jsid *beg, jsid *end)
> +{
> +    for (jsid *id = beg; id != end; ++id)

Can you make this |id < end| while you're here?  Belt-and-suspenders protection against wild overwrites if garbage comes in.

@@ +326,5 @@
> +template<class T>
> +class AutoVectorRooter : protected AutoGCRooter
> +{
> +  public:
> +    explicit AutoVectorRooter(JSContext *cx JS_GUARD_OBJECT_NOTIFIER_PARAM)

Put the guard param on a new line.

@@ +380,5 @@
> +
> +    const T &back() const { return vector.back(); }
> +
> +  protected:
> +    typedef Vector<T, 8> VectorImpl;

It might be worth considering (not here, separate bug!) making 8 configurable.  The switch-on-tag scheme made that difficult, but with virtual dispatch it'd be pretty easy.  Worthwhile?  *shrug*  Just throwing the idea out.

@@ +401,5 @@
> +
> +    const jsval *jsval_end() const { return end(); }
> +    jsval *jsval_end() { return end(); }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

This should go in the protected section -- and at the end, so as not to distort offsets in debug builds versus in optimized builds.

@@ +417,5 @@
> +    {
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

Same.

@@ +433,5 @@
> +    {
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

Same.
Attachment #564998 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 565003 [details] [diff] [review]
main patch

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

So, honestly, I kind of feel like moving every helper method into friendapi, and not even trying to answer the question of whether whatever's being done would be better done another way, is a copout.

But I guess this is a step forward at least abstraction-wise.  And it looks like the same hackaround was used for jsobj.h already.

So I guess even if this is better (if only somewhat), I am decidely meh about it.  :-|

::: content/xul/document/src/nsXULContentSink.cpp
@@ +1060,5 @@
>                // By default scripts in XUL documents have E4X turned on. We use
>                // our implementation knowledge to reuse JSVERSION_HAS_XML as a
>                // safe version flag. This is still OK if version is
>                // JSVERSION_UNKNOWN (-1),
> +              version = JS_VersionSetXML(JSVersion(version), JS_TRUE);

This could be just |true| here, and for all the other callers.  Not important to change, just might be nicer to read.  And I suspect when PRBool->bool gets applied to JSBool, someday, this would happen eventually.  So there's an earlier-is-better contention here, I think.

::: js/src/jscntxt.h
@@ +263,5 @@
>                          js::DefaultHasher<void *>,
>                          js::SystemAllocPolicy> Map;
>  
> +    /* Opaque thread-id, from NSPR's PR_GetCurrentThread(). */
> +    void                *id;

This is more a comment for the entire file than for this line, but: any chance any of the members you've rearranged here, at all, could be made private at the same time you're doing all this work?  At least a followup or something would be nice.

@@ +352,5 @@
>  
>  }
>  
>  struct JSRuntime {
> +    /* Literal table maintained by jsatom.c functions. */

jsatom.cpp

::: js/src/jsfriendapi.cpp
@@ +222,5 @@
> +    return js_DateGetMsecSinceEpoch(cx, obj);
> +}
> +
> +JS_FRIEND_API(JSBool)
> +JS_WasLastGCCompartmental(JSContext *cx)

It seems pretty sad that we have/need this API right now.

::: js/src/jsfriendapi.h
@@ +131,5 @@
>  
>  #ifdef __cplusplus
>  
> +#include "jsatom.h"
> +#include "jsvector.h"

Including headers in the middle of a file like this seems worrisome.  Is there any way to avoid that, perhaps by moving this section to the top of the file or something?

::: js/src/xpconnect/shell/xpcshell.cpp
@@ +542,5 @@
>      JSRuntime *rt;
>      uint32 preBytes;
>  
> +    rt = JS_GetRuntime(cx);
> +    preBytes = JS_GetGCParameter(rt, JSGC_BYTES);

Unify the initializations with the declarations while you're here.

@@ +1002,5 @@
>  {
>      if ((errorNumber > 0) && (errorNumber < JSShellErr_Limit))
> +        return &jsShell_ErrorFormatString[errorNumber];
> +    else
> +        return NULL;

I'm guessing this was just copypasta'd from js.msg and uses, but isn't it just an error if an error is reported with a number outside the range its reporter is supposed to handle?  I think the latter half of this should be a JS_NOT_REACHED.

::: js/src/xpconnect/src/nsXPConnect.cpp
@@ +953,2 @@
>              cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
> +                               js::GetContextGlobalObject(cx));

if (JSObject* global = js::Get...) to avoid repeating yourself?

::: js/src/xpconnect/src/xpcmaps.cpp
@@ +41,5 @@
>  /* Private maps (hashtables). */
>  
>  #include "xpcprivate.h"
>  #include "jsbit.h"
> +#include "jshash.h"

Why this?  Is this intentional?

::: js/src/xpconnect/src/xpcstring.cpp
@@ -90,5 @@
> -    JSAtom *atom;
> -    if (length == 0 && (atom = cx->runtime->atomState.emptyAtom))
> -    {
> -        return STRING_TO_JSVAL(atom);
> -    }

Mind-boggling.

::: toolkit/components/startup/nsAppStartup.cpp
@@ +706,5 @@
>  MaybeDefineProperty(JSContext *cx, JSObject *obj, const char *name, PRTime timestamp)
>  {
>    if (!timestamp)
>      return;
> +  JSObject *date = JS_NewDateObjectMsec(cx, timestamp/PR_USEC_PER_MSEC);

Spaces around the / here.
Attachment #565003 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 18

6 years ago
Comment on attachment 565003 [details] [diff] [review]
main patch

>--- a/dom/base/nsJSEnvironment.cpp
>+++ b/dom/base/nsJSEnvironment.cpp
>-    switch(cx->debugHooks->debuggerHandler(cx, script, ::JS_GetFramePC(cx, fp),
>+    JSDebugHooks *hooks = js::GetContextDebugHooks(cx);
>+    switch(hooks->debuggerHandler(cx, script, ::JS_GetFramePC(cx, fp),
>                                            &rval,
>-                                           cx->debugHooks->
>-                                           debuggerHandlerData)) {
>+                                           hooks->debuggerHandlerData)) {

Reindent, please.

>--- a/dom/workers/ListenerManager.cpp
>+++ b/dom/workers/ListenerManager.cpp
> #include "jsapi.h"
>-#include "jscntxt.h"
>+#include "jsfriendapi.h"
> #include "jsvector.h"

Can this include go now?
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb50e96dbee0
Target Milestone: --- → mozilla10
backed out for causing permanent failures in Windows PGO mochitests (1,4)

See philor's topic in tree-management
Here is some log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6981332&tree=Mozilla-Inbound
33463 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug664916.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: tagNameGetter.call is not a function at http://mochi.test:8888/tests/content/base/test/test_bug664916.html:34

https://tbpl.mozilla.org/php/getParsedLog.php?id=6981247&tree=Mozilla-Inbound
293 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug462428.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: getter.call is not a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug462428.html:34
341 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug585745.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: property descriptor's getter field is neither undefined nor a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug585745.html:29
373 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug628794.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: property descriptor's getter field is neither undefined nor a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug628794.html:29

Unfortunately there is no way to get pgo testing on Try atm, please contact releng to figure out a strategy for that.
Target Milestone: mozilla10 → ---
So far it looks like the autorooters portion of the patch is what's causing the orange. Unfortunately that's the part that's harder to bisect over. I guess I'll try converting only JSAutoObjectRooter and leave the other rooters in jscntxt.h.
Any news?
Blocks: 554088
(Assignee)

Comment 23

6 years ago
I happen to have been looking at bug 332648 today:

https://tbpl.mozilla.org/?tree=Try&rev=2198dd8ad887
https://tbpl.mozilla.org/?tree=Try&rev=1503c844adb0
I haven't had time for this lately. I did find that the part of the patch that causes orange in PGO. It's the change where I took the tag field out of AutoGCRooter and made the trace method be virtual. I don't know why that would cause problems, but it is. If someone wants to modify the patch to remove that part, I think it could land.

Ms2ger, to get useful results from tryserver, you need to change the mozconfigs to enable PGO builds. Normally we don't do PGO builds on tryserver (AFAIK), and the patch was only failing with PGO. See this page:
  https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_mozconfig

Comment 25

5 years ago
By any chance does Ms2ger wish to drive this ship home?
(Assignee)

Updated

5 years ago
Assignee: wmccloskey → Ms2ger
(Assignee)

Updated

5 years ago
Depends on: 713340
(Assignee)

Updated

5 years ago
Depends on: 713645
(Assignee)

Updated

5 years ago
(Assignee)

Updated

5 years ago
Depends on: 714264
(Assignee)

Updated

5 years ago
Depends on: 714458
(Assignee)

Comment 26

5 years ago
Created attachment 586672 [details] [diff] [review]
Part a: Move AutoLockGC to jsfriendapi.h.

Used by XPCJSRuntime.cpp.
Attachment #564998 - Attachment is obsolete: true
Attachment #565003 - Attachment is obsolete: true
Attachment #586672 - Flags: review?(igor)
(Assignee)

Comment 27

5 years ago
Created attachment 586673 [details] [diff] [review]
Part b: Use existing JSAPI functions where those are available.
Attachment #586673 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 28

5 years ago
Created attachment 586674 [details] [diff] [review]
Part c: Stop installing jsdate.h.
Attachment #586674 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 29

5 years ago
Created attachment 586675 [details] [diff] [review]
Part d: Move js_GetErrorMessage to jsfriendapi.h.
Attachment #586675 - Flags: review?(jorendorff)
(Assignee)

Comment 30

5 years ago
Created attachment 586676 [details] [diff] [review]
Part e: Move js_GetSCOffset to jsfriendapi.h.
Attachment #586676 - Flags: review?(luke)
(Assignee)

Comment 31

5 years ago
Created attachment 586677 [details] [diff] [review]
Part f: Expose structuredCloneCallbacks in jsfriendapi.h.
Attachment #586677 - Flags: review?(dvander)
(Assignee)

Comment 32

5 years ago
Created attachment 586678 [details] [diff] [review]
Part g: Expose VersionSetXML in jsfriendapi.h.
Attachment #586678 - Flags: review?(brendan)
(Assignee)

Comment 33

5 years ago
Created attachment 586679 [details] [diff] [review]
Part h: Expose debuggerHandler in jsfriendapi.h.
Attachment #586679 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #586679 - Flags: review? → review?(igor)
(Assignee)

Comment 34

5 years ago
Created attachment 586680 [details] [diff] [review]
Part i: Expose errorReporter in jsapi.h.
Attachment #586680 - Flags: review?(mrbkap)
(Assignee)

Comment 35

5 years ago
Created attachment 586681 [details] [diff] [review]
Part j: Stop using js::ContextAllocPolicy in ListenerManager.cpp.
Attachment #586681 - Flags: review?(bent.mozilla)
(Assignee)

Comment 36

5 years ago
Created attachment 586683 [details] [diff] [review]
Part k: Expose outstandingRequests in jsfriendapi.h.
Attachment #586683 - Flags: review?(christopher.leary)
(Assignee)

Comment 37

5 years ago
Created attachment 586684 [details] [diff] [review]
Part l: Use the existing JSAPI for the global object for a context.
Attachment #586684 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 38

5 years ago
Created attachment 586685 [details] [diff] [review]
Part m: Expose context's compartment in jsfriendapi.h.
Attachment #586685 - Flags: review?(jorendorff)
(Assignee)

Comment 39

5 years ago
Created attachment 586686 [details] [diff] [review]
Part n: Expose JSOPTION_UNROOTED_GLOBAL in jsfriendapi.h.
Attachment #586686 - Flags: review?(gal)
(Assignee)

Comment 40

5 years ago
Created attachment 586688 [details] [diff] [review]
Part o: Expose setActivityCallback in jsfriendapi.h.
Attachment #586688 - Flags: review?(gal)
(Assignee)

Comment 41

5 years ago
Created attachment 586689 [details] [diff] [review]
Part p: Remove AutoLockJSGC in favour of js::AutoLockGC.
Attachment #586689 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 42

5 years ago
Created attachment 586690 [details] [diff] [review]
Part q: Expose gcLock in jsfriendapi.h.
Attachment #586690 - Flags: review?(gal)
(Assignee)

Comment 43

5 years ago
Created attachment 586691 [details] [diff] [review]
Part r: Provide AutoSkipConservativeScan in jsfriendapi.h.
Attachment #586691 - Flags: review?(luke)
(Assignee)

Comment 44

5 years ago
Created attachment 586692 [details] [diff] [review]
Part s: Make JS_TRACER_INIT a function instead of a macro
Attachment #586692 - Flags: review?(wmccloskey)
(Assignee)

Comment 45

5 years ago
Created attachment 586693 [details] [diff] [review]
Part t: Provide IsContextRunningJS in jsfriendapi.h.
Attachment #586693 - Flags: review?(dvander)
(Assignee)

Comment 46

5 years ago
Created attachment 586695 [details] [diff] [review]
Part u: Provide TriggerOperationCallbacksForActiveContexts in jsfriendapi.h.
Attachment #586695 - Flags: review?(bhackett1024)
(Assignee)

Comment 47

5 years ago
Created attachment 586696 [details] [diff] [review]
Part v: Expose rt->compartments in jsfriendapi.h.
Attachment #586696 - Flags: review?(jorendorff)
(Assignee)

Comment 48

5 years ago
Created attachment 586697 [details] [diff] [review]
Part w: Move AutoVectorRooter to jsapi.h.
Attachment #586697 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #586697 - Flags: review? → review?(evilpies)
(Assignee)

Comment 49

5 years ago
Created attachment 586698 [details] [diff] [review]
Part x: Move AutoValueVector to jsapi.h.
Attachment #586698 - Flags: review?(evilpies)
(Assignee)

Comment 50

5 years ago
Created attachment 586700 [details] [diff] [review]
Part z: Move JS_CHECK_RECURSION to jsfriendapi.h.
Attachment #586700 - Flags: review?(brendan)
(Assignee)

Updated

5 years ago
Attachment #586700 - Attachment description: Part y: Move JS_CHECK_RECURSION to jsfriendapi.h. → Part z: Move JS_CHECK_RECURSION to jsfriendapi.h.
(Assignee)

Comment 51

5 years ago
Created attachment 586701 [details] [diff] [review]
Part y: Move AutoIdVector to jsapi.h.
Attachment #586701 - Flags: review?(evilpies)
(Assignee)

Comment 52

5 years ago
Created attachment 586702 [details] [diff] [review]
Part aa: Expose sizeof(JSContext) in jsfriendapi.h.
Attachment #586702 - Flags: review?(n.nethercote)
(Assignee)

Comment 53

5 years ago
Created attachment 586703 [details] [diff] [review]
Part ab: Remove jscntxt.h and dependencies from INSTALLED_HEADERS.
Attachment #586703 - Flags: review?(dmandelin)

Comment 54

5 years ago
Comment on attachment 586672 [details] [diff] [review]
Part a: Move AutoLockGC to jsfriendapi.h.

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

r+ with the comment below addressed.

::: js/src/jsfriendapi.h
@@ +460,5 @@
> +{
> +  public:
> +    explicit AutoLockGC(JSRuntime *rt = NULL
> +                        MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM);
> +    ~AutoLockGC();

Add  couple of private static functions LockGC(JSRutime *rt), UnlockGC(JSRuntime *rt) functions and use them to implement the constructors/destructors. This way the compiler can optimize the case of no locking and can also see that a reference to AutoLockGC::runtime is not leaked so it can remove a stack slot for it.
Attachment #586672 - Flags: review?(igor) → review+

Comment 55

5 years ago
Comment on attachment 586679 [details] [diff] [review]
Part h: Expose debuggerHandler in jsfriendapi.h.

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

::: js/src/jsfriendapi.cpp
@@ +515,5 @@
> +JS_FRIEND_API(JSTrapStatus)
> +CallContextDebugHandler(JSContext *cx, JSScript *script, jsbytecode *bc, Value *rval)
> +{
> +    MOZ_ASSERT(CanCallContextDebugHandler(cx));
> +    return cx->debugHooks->debuggerHandler(cx, script, bc, rval,

We should fix the preexisting bug here - after the haddler was checked for existence it could have been removed. So do not assert that the handler is not null, rather return JSTRAP_RETURN in this case.
Attachment #586679 - Flags: review?(igor) → review+

Comment 56

5 years ago
Comment on attachment 586686 [details] [diff] [review]
Part n: Expose JSOPTION_UNROOTED_GLOBAL in jsfriendapi.h.

I am not a big fan of friend APIs, we should probably turn this stuff into public apis, but other related stuff around this api is friend to, so not a problem for this patch.
Attachment #586686 - Flags: review?(gal) → review+

Comment 57

5 years ago
Comment on attachment 586688 [details] [diff] [review]
Part o: Expose setActivityCallback in jsfriendapi.h.

What is that #if 0 garbage there?

Updated

5 years ago
Attachment #586688 - Flags: review?(gal) → review+

Comment 58

5 years ago
Comment on attachment 586690 [details] [diff] [review]
Part q: Expose gcLock in jsfriendapi.h.

Not the cleanest abstraction, but I don't have any better suggestion either. Well worth the end goal of getting rid of the header include and we can cleanup the interface from there.
Attachment #586690 - Flags: review?(gal) → review+
(Assignee)

Comment 59

5 years ago
(In reply to Andreas Gal :gal from comment #57)
> Comment on attachment 586688 [details] [diff] [review]
> Part o: Expose setActivityCallback in jsfriendapi.h.
> 
> What is that #if 0 garbage there?

My todo list; it's gone entirely by the end of this patch queue. I could get rid of it, but I'd kind of rather not spend my time on the merging that will result.

(In reply to Andreas Gal :gal from comment #58)
> Comment on attachment 586690 [details] [diff] [review]
> Part q: Expose gcLock in jsfriendapi.h.
> 
> Not the cleanest abstraction, but I don't have any better suggestion either.
> Well worth the end goal of getting rid of the header include and we can
> cleanup the interface from there.

Yeah. Maybe a function of rt that returns JS_NEW_CONDVAR(rt->gcLock) would be somewhat better... I dunno.
Attachment #586702 - Flags: review?(n.nethercote) → review+
Attachment #586683 - Flags: review?(christopher.leary) → review+

Updated

5 years ago
Attachment #586680 - Flags: review?(mrbkap) → review+

Comment 60

5 years ago
Comment on attachment 586676 [details] [diff] [review]
Part e: Move js_GetSCOffset to jsfriendapi.h.

>-#include "jsclone.h"
>+#include "jscntxt.h" // structuredCloneCallbacks

I'm probably missing the grand plan, but, don't you mean 'jsfriendapi.h' ?
Attachment #586676 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #586691 - Flags: review?(luke) → review+
(Assignee)

Comment 61

5 years ago
(In reply to Luke Wagner [:luke] from comment #60)
> Comment on attachment 586676 [details] [diff] [review]
> Part e: Move js_GetSCOffset to jsfriendapi.h.
> 
> >-#include "jsclone.h"
> >+#include "jscntxt.h" // structuredCloneCallbacks
> 
> I'm probably missing the grand plan, but, don't you mean 'jsfriendapi.h' ?

It needs both jsfriendapi and jscntxt at this point. I added jsfriendapi.h in attachment 586677 [details] [diff] [review], but it makes more sense to add it here, indeed.
Attachment #586695 - Flags: review?(bhackett1024) → review+
Attachment #586674 - Flags: review?(jwalden+bmo) → review+
Attachment #586697 - Flags: review?(evilpies) → review+
Attachment #586698 - Flags: review?(evilpies) → review+
Attachment #586701 - Flags: review?(evilpies) → review+
Attachment #586692 - Flags: review?(wmccloskey) → review+
Attachment #586675 - Flags: review?(jorendorff) → review+
Attachment #586685 - Flags: review?(jorendorff) → review+
Comment on attachment 586696 [details] [diff] [review]
Part v: Expose rt->compartments in jsfriendapi.h.

In jsfriendapi.h and also in the .cpp file:
>+JS_FRIEND_API(CompartmentVector&)
>+GetRuntimeCompartments(JSRuntime *rt)

const CompartmentVector &, please.

>-        js::CompartmentVector &vector = rt->compartments;
>+        js::CompartmentVector &vector = js::GetRuntimeCompartments(rt);
>         for (JSCompartment **p = vector.begin(); p != vector.end(); ++p) {

Add const at the beginning of the line you changed, please.

It would also be best to change the for-loop to insert the `const` keyword between the two stars, like so:

          for (JSCompartment * const *p = vector.begin() ......

and I encourage you to do it even though (due to a bug in js::Vector) C++ might let you get away without it.
Attachment #586696 - Flags: review?(jorendorff) → review+
Comment on attachment 586677 [details] [diff] [review]
Part f: Expose structuredCloneCallbacks in jsfriendapi.h.

Stealing review.
Attachment #586677 - Flags: review?(dvander) → review+
Attachment #586693 - Flags: review?(dvander) → review+
Comment on attachment 586681 [details] [diff] [review]
Part j: Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

I tried to avoid XPCOM-ish stuff in these js bindings in case we decided to move them into the js engine or autogenerate them somehow. I'd really prefer not to add nsTArray here, is it just not possible to use Vector outside of the engine?

Also, this patch adds jsfriendapi.h but as far as I can tell never uses anything in it.
Attachment #586681 - Flags: review?(bent.mozilla) → review-
Attachment #586703 - Flags: review?(dmandelin) → review+
(Assignee)

Comment 65

5 years ago
We can't use js::ContextAllocPolicy outside the engine.
(Assignee)

Updated

5 years ago
Attachment #586678 - Flags: review?(brendan) → review?(bhackett1024)
Comment on attachment 586678 [details] [diff] [review]
Part g: Expose VersionSetXML in jsfriendapi.h.

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

::: js/src/jsfriendapi.cpp
@@ +501,5 @@
>  
> +JS_FRIEND_API(JSVersion)
> +VersionSetXML(JSVersion version, bool enable)
> +{
> +    VersionSetXML(&version, enable);

Can you remove the VersionSetXML from jscntxt.h and put its implementation here, then change the single caller in jscntxt.h to use this function?
Attachment #586678 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

5 years ago
Attachment #586700 - Flags: review?(brendan) → review?(mrosenberg)
Attachment #586700 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 67

5 years ago
Created attachment 587742 [details] [diff] [review]
Part j: Stop using js::ContextAllocPolicy in ListenerManager.cpp. (v2)

Like this more, perhaps?
Attachment #586681 - Attachment is obsolete: true
Attachment #587742 - Flags: review?(bent.mozilla)
Attachment #586673 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 586684 [details] [diff] [review]
Part l: Use the existing JSAPI for the global object for a context.


>         JSContext *cx = static_cast<JSContext*>(n);
>-        NS_ASSERTION(cx->globalObject, "global object NULL before unlinking");
>-        cx->globalObject = nsnull;
>+        JSAutoRequest ar(cx);
>+        NS_ASSERTION(JS_GetGlobalObject(cx), "global object NULL before unlinking");
>+        JS_SetGlobalObject(cx, NULL);

Uh oh. Does this mean we were racy before?
Attachment #586684 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 586689 [details] [diff] [review]
Part p: Remove AutoLockJSGC in favour of js::AutoLockGC.

Huzzah!
Attachment #586689 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 70

5 years ago
Created attachment 587776 [details] [diff] [review]
Stop using js::ContextAllocPolicy in ListenerManager.cpp.
Attachment #587776 - Flags: review?(sphink)
Attachment #587776 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #587742 - Attachment is obsolete: true
Attachment #587742 - Flags: review?(bent.mozilla)
Comment on attachment 587776 [details] [diff] [review]
Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

Thanks!
Attachment #587776 - Flags: review?(bent.mozilla) → review+
Comment on attachment 587776 [details] [diff] [review]
Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

I'm ok with this as is (modulo the constructor name fix), though the naming is clunky.

::: js/src/jsalloc.h
@@ +115,5 @@
>  
>      JS_FRIEND_API(void) reportAllocOverflow() const;
>  };
>  
> +class ExternallyUsableContextAllocPolicy

I don't understand the layering anymore. Could this live in js/public instead of here?

Would JSContextAllocPolicy be too confusing as a name?

@@ +120,5 @@
> +{
> +    JSContext *const cx;
> +
> +  public:
> +    ContextAllocPolicy(JSContext *cx) : cx(cx) {}

Cut & paste error -- wrong class name.
Attachment #587776 - Flags: review?(sphink) → review+
(Assignee)

Comment 73

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b2ef431401ff
https://hg.mozilla.org/mozilla-central/rev/1668811de954
https://hg.mozilla.org/mozilla-central/rev/4f665744b6e6
https://hg.mozilla.org/mozilla-central/rev/ebd92ba20fd1
https://hg.mozilla.org/mozilla-central/rev/10519108297f
https://hg.mozilla.org/mozilla-central/rev/6ffce4fb752b
https://hg.mozilla.org/mozilla-central/rev/58eef6ad44ad
https://hg.mozilla.org/mozilla-central/rev/2b2c3d92ae9f
https://hg.mozilla.org/mozilla-central/rev/f9fe45249420
https://hg.mozilla.org/mozilla-central/rev/be5a53604bc3
https://hg.mozilla.org/mozilla-central/rev/46b5d4c3b3d8
https://hg.mozilla.org/mozilla-central/rev/29a2d146c5ff
https://hg.mozilla.org/mozilla-central/rev/41ea9acca641
https://hg.mozilla.org/mozilla-central/rev/fe8240db5834
https://hg.mozilla.org/mozilla-central/rev/33d5a1f2af51
https://hg.mozilla.org/mozilla-central/rev/e798120914f7
https://hg.mozilla.org/mozilla-central/rev/35247789f332
https://hg.mozilla.org/mozilla-central/rev/fee37d705c87
https://hg.mozilla.org/mozilla-central/rev/346328bd1f2e
https://hg.mozilla.org/mozilla-central/rev/c8ba49dafe5e
https://hg.mozilla.org/mozilla-central/rev/d848942fc440
https://hg.mozilla.org/mozilla-central/rev/b2bffccbc28b
https://hg.mozilla.org/mozilla-central/rev/aa34f07b7f67
https://hg.mozilla.org/mozilla-central/rev/4b70a75df702
https://hg.mozilla.org/mozilla-central/rev/9ee7f6571842
https://hg.mozilla.org/mozilla-central/rev/cf1294a14a4c
https://hg.mozilla.org/mozilla-central/rev/9f706102086f
https://hg.mozilla.org/mozilla-central/rev/dfc618d2f28f

Thanks all!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Updated

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