In the nsJSRuntime::Init routine, there are many occurences of: Preferences::RegisterCallback(SetMemoryGCPrefChangedCallback, "javascript.options.mem.gc_high_frequency_heap_growth_min"); SetMemoryGCPrefChangedCallback("javascript.options.mem.gc_high_frequency_heap_growth_min", (void *)JSGC_HIGH_FREQUENCY_HEAP_GROWTH_MIN); where the 2nd argument to SetMemoryGCPrefChangedCallback is non-null. In these instances, the call to Preferences::RegisterCallback needs to provide a 3rd argument like: Preferences::RegisterCallback(SetMemoryGCPrefChangedCallback, "javascript.options.mem.gc_high_frequency_heap_growth_min", (void *)JSGC_HIGH_FREQUENCY_HEAP_GROWTH_MIN); otherwise, when the profile changes, it will call SetMemoryGCPrefChangedCallback with nullptr, which gets interpreted as JSGC_MAX_BYTES which will in many cases yield the assertion failure: Assertion failure: value >= rt->gcBytes, at /home/dhylands/work/B2G-unagi/mozilla-inbound/js/src/jsapi.cpp:2886

Fixes the arguments to RegisterCallback. With this patch applied, I no longer see asserts in my debug build and using STR from bug 841962. I also see the splash screen showing up 30-45 seconds sooner when this patch is applied and using a release build with STR from bug 841962.

Marking as tef? since this is really the solution for bug 841962

Why the arbitrary 10000 cap on these values? There doesn't seem to be much documentation on these values (or much in the way of module owner reviews on this code to start with), so it's hard to tell whether it actually makes sense...

That would be a question for Bill I think, since looking at the history, he added that particular limit check (which is also unrelated to this bug).

It looks like this is a developer-facing issue only based on flashing and changing update channels (in the STR of bug 841962) - in that case there's no need to block on this and a fix can be nominated for v1-train uplift. If there is user-facing slowdown here on every update, then please renominate tef?, thanks.

Comment on attachment 715922 [details] [diff] [review] Fix up arguments to RegisterCallback Sorry, I probably wasn't the right person to review this before. I added smaug, since he's reviewed stuff for me in this file. It looks fine to me. It might be nice to add a utility function that does the job of registering the callback and calling it once. That way there's less duplicate code and less room for error. Some of these values are percentages (which are allowed to exceed 100%) and some are in megabytes. I assume the 10000 upper bound exists in case people put in bogus values like 9999999. Not sure if that's worth it.

Actually, I like the idea of adding a utility function. It would move the cast to the utility function as well. I think I'll do that.

Ah, 10000MB is fine. I was worried those might be KB or B. ;)

Comment on attachment 715922 [details] [diff] [review] Fix up arguments to RegisterCallback So new patch coming?

(In reply to Olli Pettay [:smaug] from comment #9) > Comment on attachment 715922 [details] [diff] [review] > Fix up arguments to RegisterCallback > > So new patch coming? Yes

Adds Preferences::RegisterCallbackAndCall which registers a callback and calls it right away.

Instead of making 2 calls, one which is wrong, make one call to both register the callback and call it with the correct parameters.

Comment on attachment 716106 [details] [diff] [review] Part 1 - Add Preferences::RegisterCallbackAndCall s/right away/immediately for initialization./ r=me with that comment change

https://hg.mozilla.org/integration/mozilla-inbound/rev/36a1a6b81f90 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7341caf9f0

https://hg.mozilla.org/mozilla-central/rev/36a1a6b81f90 https://hg.mozilla.org/mozilla-central/rev/cd7341caf9f0

Renoming, because of Bug 847511. If we change update channels for anyone midstream this is causing some pretty bad behavior =/

(tef- still, based on assumption that changing update channels is not something and end-user will be doing. please re-nom if I'm confused.)

mvines, can we reconsider this for tef+? We want this bug uplifted to 1.0.1 branch. according to dhylands, this blocks bug 860355, which is causing our OTA smoketests to reboot with blank screens and such.

Having this bug on v1.0.1 helps in the case when somebody with a v1.0.1 Unagi wants to move to a v1.1 Unagi OTA? Bug 860355 also talks about Leo devices but I don't see how this bug would affect them ATM.

(In reply to Michael Vines [:m1] [:evilmachines] from comment #19) > Having this bug on v1.0.1 helps in the case when somebody with a v1.0.1 > Unagi wants to move to a v1.1 Unagi OTA? Bug 860355 also talks about Leo > devices but I don't see how this bug would affect them ATM. it doesnt. Leo has a different issue, and that's tracked in bug 857633. i'll update the other bug to reflect the title correctly for Unagi and 1.0.1.

This bug affects all phones, and is triggered whenever there is a channel change in the OTA updates.

How would a channel change be triggered after launch on a regular users device? Why did bug 860355 trigger a channel change? Please try to provide as much detail as you can in a single comment, making me piece the puzzle together only makes your uplift request take longer to process and less likely to be +'ed.

(In reply to Michael Vines [:m1] [:evilmachines] from comment #22) > How would a channel change be triggered after launch on a regular users > device? Why did bug 860355 trigger a channel change? I honestly have no idea *why* there was a channel change, I just know that there was one (evidence found in the logs). You'd need to talk to releng, who controls how the updates are generated to determine why a channel change occurred. As mentioned before, this particular bug affects all devices, whether they be phones, PCs, whatever. Anything that runs the update code. > Please try to provide as much detail as you can in a single comment, making > me piece the puzzle together only makes your uplift request take longer to > process and less likely to be +'ed. I've provided as much information as I know.

Ok, thanks Dave. Maybe Tony has more details on the channel change that triggered bug 860355

(In reply to Michael Vines [:m1] [:evilmachines] from comment #24) > Ok, thanks Dave. Maybe Tony has more details on the channel change that > triggered bug 860355 I'll try to clarify. our OTA prod builds are setup that a fresh flash of v1.0.1, will defacto put the user on the "beta" channel. We have to inturn run a script to move it back to "nightly" channel. And once a user is on 1.0.1 nightly, they will *not* receive any more OTA updates because it wasnt' turned on there. (since we're not hosting 1.0.1 updates) i believe that's how it works. However, if you're on 1.0.1 beta, the system sees "beta", and offeres the 1.1 beta update; since that's what does push updates. Initially, we were trying to put 1.0.1 beta users back on the 1.1 v1-train branch. So given no dogfooders flash 1.0.1 nightly builds anymore (which defactos beta channel), this is a limited user set. Hope that helps. We still want 1.0.1 and 1.1 "nightly" testers to pick up this change.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b989ea5ea6dd https://hg.mozilla.org/releases/mozilla-b2g18/rev/458e12ba2c8d https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9ed6f3f4d013 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8b2034047468

Gecko http://hg.mozilla.org/releases/mozilla-b2g18/rev/2cc6a1c89a97 Gaia f8e8f2be6803b02d6a083b2a144a80e7ebf15951 BuildID 20130416070203 Version 18.0 Verified for v1 train. Unagi