915301 - Google Maps smeared street graphics in current Nightly.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Leman Bennett [Omega]]

Attachment: gmaps-error.jpg

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130911030258

Steps to reproduce:

Google Maps in today's build streaks street graphics all over the map.

Screenshot attached.

Comment 1

[Raul Nicolae Malea]

Confirm for: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911030258 CSet: 9e9f74116749

about:buildconfig
Build Machine

bld-centos6-hp-015
Source

Built from http://hg.mozilla.org/mozilla-central/rev/9e9f74116749
Build platform
target
x86_64-unknown-linux-gnu
Build tools
Compiler 	Version 	Compiler flags
/usr/bin/ccache /tools/gcc-4.7.3-0moz1/bin/gcc 	gcc version 4.7.3 (GCC) 	-Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-unused -Wcast-align -Wno-error=uninitialized -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -std=gnu99 -fgnu89-inline -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -DNDEBUG -DTRIMMED -g -fprofile-use -fprofile-correction -Wcoverage-mismatch -O3 -fno-omit-frame-pointer
/usr/bin/ccache /tools/gcc-4.7.3-0moz1/bin/g++ 	gcc version 4.7.3 (GCC) 	-Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -Wno-error=uninitialized -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DNDEBUG -DTRIMMED -g -fprofile-use -fprofile-correction -Wcoverage-mismatch -O3 -fno-omit-frame-pointer
Configure arguments

--enable-update-channel=nightly --enable-update-packaging --with-google-api-keyfile=/builds/gapi.data --enable-crashreporter --enable-release --enable-elf-hack --enable-stdcxx-compat --enable-warnings-as-errors --enable-signmar --enable-profiling --disable-elf-hack --enable-js-diagnostics --with-ccache=/usr/bin/ccache

Comment 2

[Raul Nicolae Malea]

Hardware acceleration is enable.

Comment 3

[Alice0775 White]

Regression widnwow(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/be1053dc223b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910000748
Bad:
http://hg.mozilla.org/mozilla-central/rev/25bfaa953892
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910011434
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be1053dc223b&tochange=25bfaa953892

Regression widnwow(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a6ac4c0ab58
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909155246
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a43cf13bd6a6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909155549
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4a6ac4c0ab58&tochange=a43cf13bd6a6

Regressed by:
a43cf13bd6a6	Benjamin Bouvier — Bug 888109: Float32 general optimizations for IonMonkey: framework and arithmetic operations; r=sstangl,nbp

Comment 4

[Alice0775 White]

FYI,
Setting javascript.options.ion.content;false fixes the problem.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

The good parts: Maps use Float32 so it *could* get faster.
The bad parts: there seem to be a conversion failure somewhere.
Interesting, I will investigate this from now. I can reproduce on linux x64, so it's not OS specific. I will try to find whether it's platform specific and reduce the test case.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Coherency assertions in debug mode, for that not to happen again

Thanks to Sean for the idea: this adds a pass in debug mode that checks that all instructions specialized as Float32 are actually flowing into instructions that can handle them (consumers, instructions specialized as Float32 or ToDouble).

This patch also contains a reduced test case showing the error:
- A phi is created with type Double during ion building.
- This phi is passed as the argument of a MPassArg, later during ion building. This MPassArg is thus specialized as Double.
- During specializePhis, the Phi is respecialized as Float32, but the MPassArg is not updated.
As a matter of fact, float32 arguments flow into a function that expects doubles.

Not flagging for review yet as it could blow up some mochi tests. I am going to try first.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Temporary fix

This is ugly and gross, but this is also temporary. This regression is pretty bad and it deserves a smarter fix, that I will do in bug 915479. In the mean while, there might be correctness errors on all websites that use Float32Arrays.

In this patch, we adjust real argument types by hand, as a temporary workaround.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 803437 [details] [diff] [review]
Temporary fix

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

r=me

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Cleaner fix

This fix makes MPassArg a NoFloatPolicy, preventing it from receiving Float32 as an argument. In this case, Float32 arguments will be converted to a Double. It's cleaner than the previous patch and it even allows some code removal.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Alice, do you know if the lack of textures when zooming out is also due to the same cset?

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 803460 [details] [diff] [review]
Cleaner fix

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

Much nicer. r=me

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/348fe4a17158

Comment 13

[Alice0775 White]

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> Alice, do you know if the lack of textures when zooming out is also due to
> the same cset?

The lack of textures is caused by different cset.

I've filed Bug 915495

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/348fe4a17158

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 803432 [details] [diff] [review]
Coherency assertions in debug mode, for that not to happen again

Try returned green for the check patch:
https://tbpl.mozilla.org/?tree=Try&rev=cdde9daaba62

Comment 16

[Sean Stangl [:sstangl]]

Comment on attachment 803432 [details] [diff] [review]
Coherency assertions in debug mode, for that not to happen again

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

::: js/src/jit/IonAnalysis.cpp
@@ +911,5 @@
> +        if (mir->shouldCancel("Check Float32 coherency"))
> +            return false;
> +
> +        for (MDefinitionIterator def(*block); def; def++) {
> +            if (def->type() == MIRType_Float32) {

prefer reducing a level of indentation by changing this to a guard:

if (def->type() != MIRType_Float32)
    continue;

@@ +915,5 @@
> +            if (def->type() == MIRType_Float32) {
> +                if (def->isPassArg()) // no check for PassArg as it is broken, see bug 915479
> +                    continue;
> +
> +                for(MUseDefIterator use(*def); use; use++) {

nit: whitespace after "for"

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Also added a comment for explaining why the check makes sense.

https://hg.mozilla.org/integration/mozilla-inbound/rev/13568a3576cd

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13568a3576cd

Comment 19

[Leman Bennett [Omega]]

Should this be considered fixed with Bug 916816 describing the exact same scenario?

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Leman Bennett (Omega X) from comment #19)
> Should this be considered fixed with Bug 916816 describing the exact same
> scenario?

Yes, that bug will follow up and is tracked so this regression is correctly fixed in time for release.