Closed
Bug 1054169
Opened 10 years ago
Closed 10 years ago
Math.min bustage in Firefox Nightly 34
Categories
(Core :: JavaScript Engine, defect)
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.
Reporter | ||
Comment 1•10 years ago
|
||
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. :]
Reporter | ||
Comment 2•10 years ago
|
||
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);
Reporter | ||
Comment 3•10 years ago
|
||
don't think I can get it reduced further than "firefox nightly **** itself on the fourth argument of a math.min"
Comment 4•10 years ago
|
||
(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)
Keywords: regression,
regressionwindow-wanted
Product: Firefox → Core
Reporter | ||
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 years ago
|
||
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.
Comment 8•10 years ago
|
||
[Tracking Requested - why for this release]:
Correctness bug, breaks websites/games.
tracking-firefox34:
--- → ?
Comment 9•10 years ago
|
||
(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)
Comment 10•10 years ago
|
||
This should be fixed in the newest nightly. Can somebody confirm that?
Reporter | ||
Comment 11•10 years ago
|
||
Yes, it was fixed the other day. I'm assuming you backed out the bad patch.
Comment 12•10 years ago
|
||
(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
Reporter | ||
Comment 13•10 years ago
|
||
Still want that regression window or is it safe to say that it's been supplanted by this backout confirming the fix?
Comment 14•10 years ago
|
||
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.
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 16•10 years ago
|
||
This has regressed once again.
Comment 17•10 years ago
|
||
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.
Comment 18•10 years ago
|
||
(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.
Description
•