Green try run for these patches: https://tbpl.mozilla.org/?tree=Try&rev=6b6bb96ea202

Comment on attachment 8333518 [details] [diff] [review] Override the default JIT flags on RuntimeOptions instead of ContextOptions Review of attachment 8333518 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscompartment.h @@ +161,5 @@ > + /* Unlike the other options that can be overriden on a per compartment > + * basis, the default value for the typeInference option is stored on the > + * compartment's type zone, rather than the current JSContext. Type zones > + * copy this default value over from the current JSContext when they are > + * created. The stuff about JSContext here is wrong, no? It should be JSRuntime unless I'm confused.

Comment on attachment 8333521 [details] [diff] [review] Update consumers (dom) Review of attachment 8333521 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ +770,5 @@ > useAsmJS = false; > } > } > > + JS::RuntimeOptionsRef(cx).setTypeInference(useTypeInference) Even though it's probably benign, it really doesn't make sense to have a per-JSContext callback that sets flags on the runtime. This should be moved into a callback on XPCJSRuntime. ::: js/xpconnect/src/nsXPConnect.cpp @@ +426,5 @@ > if (NS_FAILED(xr->GetInSafeMode(&safeMode)) || safeMode) > return; > > // Finally, do the damn assert. > + MOZ_ASSERT(RuntimeOptionsRef(cx).typeInference()); This check is intended to make sure that TI is actually enabled on a given global, not on the runtime. As such, this is incorrect, and needs to be reworked.

(In reply to Bobby Holley (:bholley) from comment #10) > Comment on attachment 8333521 [details] [diff] [review] > Update consumers (dom) > > Review of attachment 8333521 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/nsJSEnvironment.cpp > @@ +770,5 @@ > > useAsmJS = false; > > } > > } > > > > + JS::RuntimeOptionsRef(cx).setTypeInference(useTypeInference) > > Even though it's probably benign, it really doesn't make sense to have a > per-JSContext callback that sets flags on the runtime. This should be moved > into a callback on XPCJSRuntime. > That sounds reasonable. However, it turns out that: 1. The existing code already does exactly that for the following functions: - JS_SetParallelParsingEnabled - JS_SetParallelIonCompilationEnabled - JS_SetGlobalJitCompilerOption - JS_SetGlobalJitCompilerOption 2. The above functions take a JSContext, not a JSRuntime, so to move them over to XPCJSRuntime we have to refactor them first. Given that, I think it makes sense to file a followup bug for this. > ::: js/xpconnect/src/nsXPConnect.cpp > @@ +426,5 @@ > > if (NS_FAILED(xr->GetInSafeMode(&safeMode)) || safeMode) > > return; > > > > // Finally, do the damn assert. > > + MOZ_ASSERT(RuntimeOptionsRef(cx).typeInference()); > > This check is intended to make sure that TI is actually enabled on a given > global, not on the runtime. As such, this is incorrect, and needs to be > reworked. We agreed on IRC to expose a FullJitEnabled function on jsfriendapi and assert against that instead.

New patch with review comments addressed (except for the comment on JSOptionChangedCallback). Followup bug for moving per runtime option to a callback in XPCJSRuntime: https://bugzilla.mozilla.org/show_bug.cgi?id=940676 Try run for this patch: https://tbpl.mozilla.org/?tree=Try&rev=adcbdaeb89d0

Comment on attachment 8334835 [details] [diff] [review] Patch Review of attachment 8334835 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with comments addressed, and assuming (per IRC discussion) that the followup happens tomorrow. ::: js/src/jsfriendapi.cpp @@ +1091,5 @@ > return gDOMProxyShadowsCheck; > } > > bool > +js::FullJitEnabled(JSContext *cx) This should take a JSCompartment directly. No need to add new APIs that depend on JSContext. ::: js/xpconnect/src/nsXPConnect.cpp @@ +385,5 @@ > #ifdef DEBUG > #include "mozilla/Preferences.h" > #include "nsIXULRuntime.h" > static void > +CheckFullJitEnabled(JSContext *cx, const JSClass *clasp, nsIPrincipal *principal) This doesn't need to operate on cx anymore, right? It seems like we could just take one argument, |HandleObject global|. We can pull clasp off of that with GetObjectClass, and can pull the principal off that with GetObjectPrincipal. You can get the runtime with XPCJSRuntime::Get()->GetJSRuntime() or something like that.

Wait, crap. This actually can't land without bug 940676, because it breaks the chrome/content distinction as Eddy pointed out in bug 940676. :-(

Essentially the same patch as before, but with the changes from bug 940676 merged in. Unfortunately, these merged in changes patch causes all xpcshell tests to fail. The problem seems to be related with how we deal with the type inference setting: - The default type inference setting is stored on RuntimeOptions - Each zone has its own type inference setting, which it copies from RuntimeOptions on creation. - Each compartment has an override for the type inference setting. By default, it just uses the value stored on its zone, but optionally, the override can force this value to true or false. When I set it to override for the type inference setting on a compartment to anything other than the default value stored on its zone, I experience a gc crash (see FIXME comment in the patch). Is there some invariant I'm overlooking here? I've been struggling to figure out why this breaks. Here's a typical stack trace: http://pastebin.mozilla.org/3671853

Also flagging Jan for info since he lives in my time zone and might be able to answer sooner.

(In reply to Eddy Bruel [:ejpbruel] from comment #16) > When I set it to override for the type inference setting on a compartment to > anything other than the default value stored on its zone, I experience a gc > crash (see FIXME comment in the patch). Is there some invariant I'm > overlooking here? I've been struggling to figure out why this breaks. Since zones landed, TI has been enabled per zone. Do we really want/need compartments to override this? The TI flag controls quite a lot of code in the engine and I'm not surprised this breaks some invariants. If we really want this we should talk to Bill and Brian, but if there's no good reason I'd really prefer this to be a per-Zone flag like it's now.

(In reply to Jan de Mooij [:jandem] from comment #18) > (In reply to Eddy Bruel [:ejpbruel] from comment #16) > > When I set it to override for the type inference setting on a compartment to > > anything other than the default value stored on its zone, I experience a gc > > crash (see FIXME comment in the patch). Is there some invariant I'm > > overlooking here? I've been struggling to figure out why this breaks. > > Since zones landed, TI has been enabled per zone. Do we really want/need > compartments to override this? The TI flag controls quite a lot of code in > the engine and I'm not surprised this breaks some invariants. > > If we really want this we should talk to Bill and Brian, but if there's no > good reason I'd really prefer this to be a per-Zone flag like it's now. The reason we want to override the JIT flags on a per compartment basis is that we want to enable the JIT for content compartments, but disable it for chrome compartments. The reason is apparently that right now, enabling the JIT for chrome compartments breaks xpcshell. Since content and chrome compartments can share the same zone, enabling/disabling the type inference flag on a per zone basis is not good enough. Note that I only have a second hand understanding of what is going on here (I'm essentially just parrotting what bholley told me), so I'm open to alternative solutions. I *did* notice that if instead of overriding the value on the zone, I override the value on the runtime, and store that into the zone, the gc problems go away, but as I explained above, that's not an adequate solution.

After talking to jandem on irc, I'm starting to think that I have misunderstood something somewhere. What I would like to do is make the type inference flag a per zone setting rather than a per compartment setting. From what I understood from bholley, this is not possible, because chrome and content compartments share zones. But if that is true, wouldn't that lead to the following contradiction? - Type inference doesn't work for chrome - The type inference flag is per zone - Chrome and content compartments share zones - Consequently, type inference is either enabled for both, or disabled for both - Type inference is definitely enabled for content - But then type inference must be enabled for chrome. Contradiction! I know we currently have separate chrome and content prefs for type inference, and that we use nsJSContext::JSOptionChangedCallback in dom/base/nsJSEnvironment.cpp to keep the typeInference field on the ContextOptions of each JSContext in sync with the appropriate pref. I'm assuming the idea is that chrome has it's own JSContext, which tracks the chrome pref, whereas the JSContext(s) for content track the content pref. However, the typeInference field on ContextOptions is currently only used to initialize the inferenceEnabled field on TypeZone when it is first initialized. In other words, if a chrome and content compartment share zones, the inferenceEnabled flag is set to the typeInference flag of whatever context we use when the first compartment was created. If the content compartment is created first, type inference must be enabled for chrome. Conversely, if the chrome compartment is created first, type inference is disabled for content. I should also note that in the above patch, *not* overriding the type inference flag to false for chrome does not lead to failing xpcshell tests, which seems to contradict that type inference currently does not work for chrome. In my mind, there are a couple of explanations: - Type inference is not enabled for content (very unlikely) - Type inference actually works for chrome - Chrome and content do not actually share zones

After talking to jandem on irc, I'm starting to think that I have misunderstood something somewhere. What I would like to do is make the type inference flag a per zone setting rather than a per compartment setting. From what I understood from bholley, this is not possible, because chrome and content compartments share zones. But if that is true, wouldn't that lead to the following contradiction? - Type inference doesn't work for chrome - The type inference flag is per zone - Chrome and content compartments share zones - Consequently, type inference is either enabled for both, or disabled for both - Type inference is definitely enabled for content - But then type inference must be enabled for chrome. Contradiction! I know we currently have separate chrome and content prefs for type inference, and that we use nsJSContext::JSOptionChangedCallback in dom/base/nsJSEnvironment.cpp to keep the typeInference field on the ContextOptions of each JSContext in sync with the appropriate pref. I'm assuming the idea is that chrome has it's own JSContext, which tracks the chrome pref, whereas the JSContext(s) for content track the content pref. However, the typeInference field on ContextOptions is currently only used to initialize the inferenceEnabled field on TypeZone when it is first initialized. In other words, if a chrome and content compartment share zones, the inferenceEnabled flag is set to the typeInference flag of whatever context we use when the first compartment was created. If the content compartment is created first, type inference must be enabled for chrome. Conversely, if the chrome compartment is created first, type inference is disabled for content. I should also note that in the above patch, *not* overriding the type inference flag to false for chrome does not lead to failing xpcshell tests, which seems to contradict that type inference currently does not work for chrome. In my mind, there are a couple of explanations: - Type inference is not enabled for content (very unlikely) - Type inference actually works for chrome - Chrome and content do not actually share zones

Bobby, do you have any insight in comment #21?

Type inference is enabled per-zone. It has to be that way because of how the underlying data structures are set up. Whether it's enabled for the zone is determined by the context flags for the context that creates the zone. Right now, the system zone is always created with a chrome context. The system zone can have compartments in it that have non-system principles. But that doesn't affect whether TI is enabled.

(In reply to Bill McCloskey (:billm) from comment #23) > Type inference is enabled per-zone. It has to be that way because of how the > underlying data structures are set up. Whether it's enabled for the zone is > determined by the context flags for the context that creates the zone. Right > now, the system zone is always created with a chrome context. The system > zone can have compartments in it that have non-system principles. But that > doesn't affect whether TI is enabled. Right, that makes sense. Can I conclude from that that overriding the type inference flag on a per-compartment basis will not be possible without significant changes to the underlying data structures? The system zone is created as a result of creating a compartment on the chrome context, is it not? We pass the options for that compartment to js::NewCompartment. What if, when we know we are creating a global for chrome, we set the override for the type inference flag to false in xpc::CreateGlobalObject before calling JS_NewGlobalObject, and then initialize the type inference flag on the zone in TypeZone::init using the overridden flag on the compartment options, instead of the flag on the context (or with the above patch, on the runtime). This should work, since if I understand you correctly, all chrome compartments use the system zone, and content compartments can share the system zone with chrome compartments, but not vice versa. As long as we manage to disable type inference for the system zone, things should work correctly. The only caveat to the above approach would be that content compartments that share the system zone with chrome compartments will have type inference disabled, regardless of any flags or override set on the runtime or their compartment, respectively. Obviously, this is all terribly messy, but I can't think of a better solution right now that doesn't require us to refactor the TI data structures. Since this code can be removed again as soon as we can enable the JIT for chrome, I'd argue it's worth getting this stuff in.

(In reply to Eddy Bruel [:ejpbruel] from comment #21) > - Type inference doesn't work for chrome > - The type inference flag is per zone > - Chrome and content compartments share zones > - Consequently, type inference is either enabled for both, or disabled for > both > - Type inference is definitely enabled for content > - But then type inference must be enabled for chrome. Contradiction! The issue is that the current semantics are just sloppy. They don't implement anything all that meaningful. > I know we currently have separate chrome and content prefs for type > inference, and that we use nsJSContext::JSOptionChangedCallback in > dom/base/nsJSEnvironment.cpp to keep the typeInference field on the > ContextOptions of each JSContext in sync with the appropriate pref. I'm > assuming the idea is that chrome has it's own JSContext, which tracks the > chrome pref, whereas the JSContext(s) for content track the content pref. Kinda sorta, but not really. There are many JSContexts. The notion of "chrome" here isn't even well-defined. There's "chrome" in the sense of System Principal, and "chrome" in the sense of a docshell of type chrome. It's all very confusing. I just started typing an explanation of how this actually works, and it started ballooning. I think it'd probably be better to explain over vidyo tomorrow, if you want to know. > > However, the typeInference field on ContextOptions is currently only used to > initialize the inferenceEnabled field on TypeZone when it is first > initialized. In other words, if a chrome and content compartment share > zones, the inferenceEnabled flag is set to the typeInference flag of > whatever context we use when the first compartment was created. If the > content compartment is created first, type inference must be enabled for > chrome. Conversely, if the chrome compartment is created first, type > inference is disabled for content. Exactly. > In my mind, there are a couple of explanations: > - Type inference is not enabled for content (very unlikely) We have an assertion against that, which you've seen. > - Type inference actually works for chrome It does, sometimes. > - Chrome and content do not actually share zones They mostly don't, but it is possible to construct situations where they do. In the situations where they do, we're presumably running with TI enabled, because otherwise the assertion would fire. (In reply to Eddy Bruel [:ejpbruel] from comment #24) > Right, that makes sense. Can I conclude from that that overriding the type > inference flag on a per-compartment basis will not be possible without > significant changes to the underlying data structures? Correct. I don't think it is a worthy goal to strive for. > The system zone is created as a result of creating a compartment on the > chrome context, is it not? There is no such thing as "the chrome context". The system zone is created the first time someone calls JS_NewGlobalObject with JS::SystemZone as the zone specifier: http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#2519 I don't know where that happens at the present moment, and what flags we end up with. > This should work, since if I understand you correctly, all chrome > compartments use the system zone No, that's not the case. The system zone is used for JSMs and stuff, but chrome DOM windows (of which there are several for every tab, on the other side of the 'top' boundary) have their own zone. > and content compartments can share the system zone with chrome compartments Yes, there are sometimes unprivileged compartments in the system zone. Sandboxes, for example, end up in the system zone unless you pass an explicit sameZoneAs. > , but not vice versa. No. Chrome compartments are sometimes created outside the system zone (imagine loading about:home in an iframe in a content webpage). I know this is all terribly confusing. I can help you understand the whole setup if you like, but it's probably not worth your time. I don't think we're going to get to a setup where we slice the TI boundary perfectly along chrome and content lines. Is XPCShell really the only situation where we care about having TI disabled? If so, it seems like we could just disable TI runtime-wide in XPCShellImpl.cpp, enable it runtime-wide for everything else, and wash our hands out this override business. If there are other stakes, we need to get a clearer handle on them, because it's going to dictate what we do here. In particular, sandboxes are currently allocated in the SystemZone (unless you specify a zone with sameZoneAs). So to have TI for sandboxes in the general case, we need to either enable TI for the system zone, or put sandboxes in a different zone by default.

Eddy, do you think this will happen in a few weeks or so? If not I'm going to work on bug 931861 and bug 929374.

(In reply to Jan de Mooij [:jandem] from comment #26) > If not I'm going to work on bug 931861 and bug 929374. The problem with that is that there's still bug 776798 :( So let me know if I can help here, as this really is the way to go.

(In reply to Jan de Mooij [:jandem] from comment #27) > (In reply to Jan de Mooij [:jandem] from comment #26) > > If not I'm going to work on bug 931861 and bug 929374. > > The problem with that is that there's still bug 776798 :( So let me know if > I can help here, as this really is the way to go. The code is all done, more or less. It just needs somebody to own getting it landed.

(In reply to Bobby Holley (:bholley) from comment #28) > The code is all done, more or less. It just needs somebody to own getting it > landed. OK, I'd be happy to help with that...

(In reply to Jan de Mooij [:jandem] from comment #29) > (In reply to Bobby Holley (:bholley) from comment #28) > > The code is all done, more or less. It just needs somebody to own getting it > > landed. > > OK, I'd be happy to help with that... \o/ Talk to Eddy.

I just landed bug 929374 (enabling Ion/TI for chrome) and had to fix a number of Mochitest-BC problems in the process. So hopefully that paved the way for fixing this one. If bug 929374 sticks I'll rebase this patch and try to get it in (with permission from Eddy).

Rebased and made some changes. Still WIP but Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=4ecaa8d7b875 ASAN Moth failure looks real but should be easy to fix by tweaking the trusted stack size in ASAN builds. I also removed the separate chrome/content prefs for Baseline/Ion/TI. Now there's just a single pref for each of them, this nicely simplifies things as we no longer have to override things per compartment.

Eddy, this patch is based on your patch, but with a number of changes. See also comment 32. Try is green other than that ASAN issue; I'll post a patch for that but in a separate bug. I'm fine with adding your name as author and r=jandem, not sure how we do such things usually. Let me know what you think; I don't really care as long as we fix this bug :)

Comment on attachment 8377779 [details] [diff] [review] Patch Bobby, can you review the xpconnect/ and dom/ changes? Also r? Ben for the worker changes. We're now passing both RuntimeOptions and ContextOptions; this should be temporary as we move more stuff from ContextOptions to RuntimeOptions/CompartmentOptions.

Comment on attachment 8377779 [details] [diff] [review] Patch Review of attachment 8377779 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this! I didn't look at the workers changes, and didn't look at the js.cpp changes all that closely. r=bholley on the rest with comments addressed. ::: js/xpconnect/src/XPCShellImpl.cpp @@ +1489,5 @@ > printf("JS_NewContext failed!\n"); > return 1; > } > > // Ion not enabled yet here because of bug 931861. Didn't we fix that bug? Do we have a followup on file for turning it on? ::: js/xpconnect/src/nsXPConnect.cpp @@ +399,5 @@ > CreateGlobalObject(JSContext *cx, const JSClass *clasp, nsIPrincipal *principal, > JS::CompartmentOptions& aOptions) > { > + // Make sure that the full JIT is enabled for everything. > + CheckFullJitEnabled(JS_GetRuntime(cx), clasp, principal); Given that this stuff is simple now, I think we can just rip out this verification machinery. It was basically designed to make sure that this stuff came out right for a given global object, but given that this stuff is runtime-wide now, I don't think we need it. Let's get rid of it, and drop the jsfriendapi changes.

(In reply to Bobby Holley (:bholley) from comment #35) > Didn't we fix that bug? Do we have a followup on file for turning it on? I tested xpcshell with this patch and we run with Ion + TI enabled, because we read the browser prefs when we create the XPCJSRuntime. Is this expected and can we rely on that? If yes I can remove these two lines right? // Ion not enabled yet here because of bug 931861. JS::ContextOptionsRef(cx).setBaseline(true);

With bholley's comments addressed, except for the pending needinfo in comment 36. bent, I also changed the patch so that we no longer have separate RuntimeOptions for chrome/content for workers, as we discussed a bit yesterday. Atm they are exactly the same and I don't think we want to split them anytime soon, as the rest of the browser uses a single JSRuntime for chrome/content. Even if we want this later, it will be very easy to add back.

(In reply to Jan de Mooij [:jandem] from comment #36) > If yes I can remove these two lines right? > > // Ion not enabled yet here because of bug 931861. > JS::ContextOptionsRef(cx).setBaseline(true); Yeah that sounds right.

Comment on attachment 8379064 [details] [diff] [review] Patch v2 Review of attachment 8379064 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the changes in dom/workers. Thanks!

Comment on attachment 8379064 [details] [diff] [review] Patch v2 It's been a week, switching review to avoid bitrot. Luke: bent reviewed the worker changes and bholley reviewed most of the rest but didn't look closely at shell/js.cpp, so if you could look at that part in particular that would be great.

Comment on attachment 8379064 [details] [diff] [review] Patch v2 \o/ \o/

(In reply to Jan de Mooij [:jandem] from comment #33) > Created attachment 8377779 [details] [diff] [review] > Patch > > Eddy, this patch is based on your patch, but with a number of changes. See > also comment 32. Try is green other than that ASAN issue; I'll post a patch > for that but in a separate bug. > > I'm fine with adding your name as author and r=jandem, not sure how we do > such things usually. Let me know what you think; I don't really care as long > as we fix this bug :) You did me a huge favor by taking this bug off my plate. I felt responsible for finishing it, but couldn't figure out how to fix the failing tests on try, since I couldn't reproduce them locally. So by all means, put yourself down as author :)

Still seeing the ASAN test failure I mentioned in another comment: https://tbpl.mozilla.org/?tree=Try&rev=d1c9c8306087 I think we're Baseline-compiling that test now, and we're using a bit more stack space (probably caused by the eval not being optimized/inlined). This patch bumps the kTrustedScriptBuffer value we use for ASAN builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/af372888bbbf https://hg.mozilla.org/integration/mozilla-inbound/rev/802aa43ae8cc

backed out 802aa43ae8cc for causing errors like https://tbpl.mozilla.org/php/getParsedLog.php?id=35278195&tree=Mozilla-Inbound on Win PGO

(In reply to Carsten Book [:Tomcat] from comment #45) > backed out 802aa43ae8cc for causing errors like > https://tbpl.mozilla.org/php/getParsedLog.php?id=35278195&tree=Mozilla- > Inbound on Win PGO Very silly, the trusted script buffer is too small with Windows PGO builds. We can store 10 frames instead of the expected 11 :( Will Try-server a patch to fix that.

kTrustedScriptBuffer is pretty small on Windows compared to the other platforms. This patch increases it from 40k to 50k. I'm pretty sure this will fix the Win32 PGO test failure, PGO Try run: https://tbpl.mozilla.org/?tree=Try&rev=1399147c6ba7

So I'm guessing this should have been marked leave-open after the backout.

bholley, hanks for the quick reviews! Landed the patch for the Win32 PGO Moth orange: https://hg.mozilla.org/integration/mozilla-inbound/rev/83115f0687d2 There are two problems left before I can reland the main patch: (1) ASan builds frequently timeout when running M-bc. The tests seem to be close to OOM without the patch (see also bug 939036) and this patch somehow pushes it over it. I'm testing possible fixes for this on Try. (2) Jetpack timeouts with Windows PGO builds, probably another MSVC bug; will try to repro this locally.

Let's try this again, after fixing the 2 PGO issues and ASan OOM: https://hg.mozilla.org/integration/mozilla-inbound/rev/508848ad378a Try: https://tbpl.mozilla.org/?tree=Try&rev=3868aa83d33e

Backed out at the request of jsmith for causing bug 978450: remote: https://hg.mozilla.org/mozilla-central/rev/c8bea55437c1

To be clear, only the last part (508848ad378a) was backed out.

for reference, 508848ad378a causes a 2.48% performance regression in the ts, paint test on the osx 10.6 platform.

I'm about to give up on this bug, it's cursed. It exposed an existing bug on b2g but I've no idea how to debug this. I don't have a b2g device and setting up a b2g build environment takes many hours..

(In reply to Jan de Mooij [:jandem] from comment #57) > I'm about to give up on this bug, it's cursed. It exposed an existing bug on > b2g but I've no idea how to debug this. I don't have a b2g device and > setting up a b2g build environment takes many hours.. Please don't do that! :-( nbp, can you help out jandem here on the b2g stuff? This is important.

(In reply to Bobby Holley (:bholley) from comment #58) > (In reply to Jan de Mooij [:jandem] from comment #57) > > I'm about to give up on this bug, it's cursed. It exposed an existing bug on > > b2g but I've no idea how to debug this. I don't have a b2g device and > > setting up a b2g build environment takes many hours.. > > Please don't do that! :-( > > nbp, can you help out jandem here on the b2g stuff? This is important. Yes, as soon as I can get my hand on a Debug build with symbols. So far I have been stuck on the configure because of a TestManifest error in our build system. Right now I am trying to disable all the tests to see if I can make build, and hopefully flash & debug after …

(In reply to Nicolas B. Pierron [:nbp] from comment #59) > Yes, as soon as I can get my hand on a Debug build with symbols. FYI, when I encountered bug 978450, I couldn't reproduce it on a debug build.

On an Unagi: I was able to reproduce this bug: - With an opt-build with Nuwa enabled. I was unable to reproduce it: - with a debug build. - with an opt-build without Nuwa. Sadly, Nuwa prevent stopping the forked process when is it spawned, so I am unable to hook a gdb on the child process yet.

(In reply to Bobby Holley (:bholley) from comment #58) > (In reply to Jan de Mooij [:jandem] from comment #57) > > I'm about to give up on this bug, it's cursed. It exposed an existing bug on > > b2g but I've no idea how to debug this. I don't have a b2g device and > > setting up a b2g build environment takes many hours.. > > Please don't do that! :-( I was able to build b2g on Linux (went a lot better than on OS X 10.9) and nbp sent me a Unagi that should arrive tomorrow, so this is not over yet.

(In reply to Jan de Mooij [:jandem] from comment #62) > I was able to build b2g on Linux (went a lot better than on OS X 10.9) and > nbp sent me a Unagi that should arrive tomorrow, so this is not over yet. \o/ Let me know if there are any resources you need that you're not getting. This needs to happen at some point, and will invariably require overcoming a bunch of mysterious hurdles like the ones you've encountered. So it'd a shame if we let the results of your valiant efforts slip away here.

(In reply to Nicolas B. Pierron [:nbp] from comment #61) > On an Unagi: > > I was able to reproduce this bug: > - With an opt-build with Nuwa enabled. > > I was unable to reproduce it: > - with a debug build. Debug builds turn off dom.ipc.processPrelaunch.enabled, which may be related — specifically, I think this also effectively disables Nuwa. This also means that debug+prelaunch is untested and might break in unrelated ways; see, for example, bug 978419. This is where that happens: https://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#693 Also, we have an |edit-prefs.sh| script in the B2G repo, which might be an easier way to change the prefs than editing that file and reflashing. > - with an opt-build without Nuwa. > > Sadly, Nuwa prevent stopping the forked process when is it spawned, so I am > unable to hook a gdb on the child process yet. There's an ifdef'ed out sleep in the right place to do that: https://mxr.mozilla.org/mozilla-central/source/mozglue/build/Nuwa.cpp#1564

(In reply to Jed Davis [:jld] from comment #64) > (In reply to Nicolas B. Pierron [:nbp] from comment #61) > Also, we have an |edit-prefs.sh| script in the B2G repo, which might be an > easier way to change the prefs than editing that file and reflashing. > > > - with an opt-build without Nuwa. > > > > Sadly, Nuwa prevent stopping the forked process when is it spawned, so I am > > unable to hook a gdb on the child process yet. > > There's an ifdef'ed out sleep in the right place to do that: > > https://mxr.mozilla.org/mozilla-central/source/mozglue/build/Nuwa.cpp#1564 I think Nuwa-forked child should follow the convention to use the environment variable. Bug 979741 is opened for this.

(In reply to Jed Davis [:jld] from comment #64) > Debug builds turn off dom.ipc.processPrelaunch.enabled, which may be related > — specifically, I think this also effectively disables Nuwa. This also > means that debug+prelaunch is untested and might break in unrelated ways; > see, for example, bug 978419. This is where that happens: > > https://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#693 Thanks, testing on an opt-build, the fact to set "dom.ipc.processPrelaunch.enabled" to false Hide this error. So I guess the opposite might work too to reproduce this issue on a debug build (not tested yet)

Bug 980067 is in, let's see what happens this time: https://hg.mozilla.org/integration/mozilla-inbound/rev/45ac7b7d7466

Could this have caused a regression in Tp5 Optimized (Private Bytes) on Linux? It got blamed on bug 946658, but that has been backed out and the regression didn't go away. This bug was right after bug 946658 in the pushlog. See the graph http://graphs.mozilla.org/graph.html#tests=[[257,131,35]]&sel=1394102638000,1394275438000&displayrange=7&datatype=running

> Could this have caused a regression in Tp5 Optimized (Private Bytes) on Linux? A priori yes, since it turns on JITs for cases where they're not turned on now, and JITs do use memory (e.g. for the output!).

This caused a new warning to show up in debug builds: In file included from /home/ben/code/moz/builds/d64/js/src/shell/Unified_cpp_js_src_shell0.cpp:2:0: /home/ben/code/moz/inbound/js/src/shell/js.cpp: In function ‘bool Options(JSContext*, unsigned int, jsval*)’: /home/ben/code/moz/inbound/js/src/shell/js.cpp:702:24: warning: variable ‘oldRuntimeOptions’ set but not used [-Wunused-but-set-variable] JS::RuntimeOptions oldRuntimeOptions = JS::RuntimeOptionsRef(cx);

Comment on attachment 8394696 [details] [diff] [review] kill-warning-jscpp.patch Review of attachment 8394696 [details] [diff] [review]: ----------------------------------------------------------------- Not to be pedantic, but bug 972817 introduced this :)