Closed Bug 1054169 Opened 10 years ago Closed 10 years ago

Math.min bustage in Firefox Nightly 34

Categories

(Core :: JavaScript Engine, defect)

34 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1053074
Tracking Status
firefox34 - ---

People

(Reporter: grantgalitz, Unassigned)

Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release) Build ID: 20140814005302 Steps to reproduce: Ran either gameboy-online or Iodine GBA with audio enabled. Actual results: The white noise channel 4 audio was glitched and was usually silent when it should be playing. A counter in the code went negative when that should have been impossible. Expected results: Correct counter logic so the channel would play correctly.
Bustage occurs on Math.min with 4 arguments. Nesting 2 argument math.mins to workaround the 4 argument version is successful in avoiding this bug. Specifically in the IodineGBA code, we have: https://github.com/grantgalitz/IodineGBA/blob/master/IodineGBA/IodineGBA/GameBoyAdvanceSoundCore.js#L322 this.audioClocksUntilNextEventCounter = this.audioClocksUntilNextEvent = Math.min(this.channel1.FrequencyCounter | 0, this.channel2.FrequencyCounter | 0, this.channel3.counter | 0, this.channel4.counter | 0) | 0; If this gets reduced to: this.audioClocksUntilNextEventCounter = this.audioClocksUntilNextEvent = Math.min(Math.min(Math.min(this.channel1.FrequencyCounter | 0, this.channel2.FrequencyCounter | 0), this.channel3.counter | 0), this.channel4.counter | 0) | 0; the bug no longer occurs. The math.min specifically mishandles the 4th argument. This is a regression from a week or two ago I think of the top of my head. FYI, this affects the gbemu bench, as it means firefox nightly is miscomputing the emulated audio channel. :]
For the code Firefox Nightly glitches on in the gameboy-online project: https://github.com/grantgalitz/GameBoy-Online/blob/master/js/GameBoyCore.js#L5578 this.audioClocksUntilNextEventCounter = this.audioClocksUntilNextEvent = Math.min(this.channel1FrequencyCounter, this.channel2FrequencyCounter, this.channel3Counter, this.channel4Counter);
don't think I can get it reduced further than "firefox nightly **** itself on the fourth argument of a math.min"
(In reply to Grant Galitz from comment #3) > don't think I can get it reduced further than "firefox nightly **** itself > on the fourth argument of a math.min" I'm confused. Math.min picks the *lowest* argument, so it going negative seems unsurprising by itself, if you pass it negative arguments? Just running a few checks like: Math.min(10, 20, 30, 50) and Math.min(10, 20, 30, -10) the fourth argument seems to work fine ( Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0; Nightly from the 13th ) Can you reduce the testcase from "running website X" (esp. in case website X changes whatever code they use) to a smaller case? Perhaps you can give input values which reproduce the problem? Alternatively, could you try to find a regression window using mozregression? ( http://mozilla.github.io/mozregression/ )
Component: Untriaged → JavaScript Engine
Flags: needinfo?(grantgalitz)
Product: Firefox → Core
The value from the math.min is then used to subtract from the counters. It picking a higher than anticipated value causes it to go negative. It's already affected gameboy-online and iodinegba with different enough code and math.min 4 argument usage that the bustage seems to not afflict only 1 kind of code. This needs a standalone test page, yes.
Flags: needinfo?(grantgalitz)
Make sure the code tested gets optimized by the JIT. There might be funky optimizations a one time call to Math.min might no go through.
Hannes could this be from bug 1042823?
Flags: needinfo?(hv1989)
[Tracking Requested - why for this release]: Correctness bug, breaks websites/games.
(In reply to Jan de Mooij [:jandem] from comment #7) > Hannes could this be from bug 1042823? I'm pretty sure it was introduced by that bug. The fuzzer bug is bug 1053074. I'm gonna to backout.
Flags: needinfo?(hv1989)
This should be fixed in the newest nightly. Can somebody confirm that?
Yes, it was fixed the other day. I'm assuming you backed out the bad patch.
(In reply to Grant Galitz from comment #11) > Yes, it was fixed the other day. I'm assuming you backed out the bad patch. Yes indeed. Thanks for confirming.
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Still want that regression window or is it safe to say that it's been supplanted by this backout confirming the fix?
No need to get that regression window. I'm pretty sure this is caused by: https://hg.mozilla.org/integration/mozilla-inbound/rev/c01dea53dd60 Nothing else touched ionmonkey Math.min/max implementation during that time.
Not tracking as this is duplicate.
This has regressed once again.
File a new bug, and ideally could you produce a reduced testcase? Clearly there's something different about your code, versus the testcases that were landed before bug 1053074 was backed out.
(In reply to Grant Galitz from comment #16) > This has regressed once again. Hey Grant, like mentioned above, can you create a new bug report for this with additional information? 1) Which version of Firefox where you running? (nightly, dev tools, beta ... release) 2) A reduced testcase would be easiest 2) Clear steps to reproduce so the person investigating can reproduce. Please cc me on it or mention the bug number here. Thanks.
You need to log in before you can comment on or make changes to this bug.