Closed Bug 915301 Opened 6 years ago Closed 6 years ago

Google Maps smeared street graphics in current Nightly.

Categories

(Core :: JavaScript Engine, defect)

26 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: lh.bennett, Assigned: bbouvier)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image 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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8 → All
Hardware: x86_64 → All
Hardware acceleration is enable.
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
Assignee: nobody → general
Blocks: 888109
Component: Untriaged → JavaScript Engine
Keywords: regression
Product: Firefox → Core
Version: Trunk → 26 Branch
FYI,
Setting javascript.options.ion.content;false fixes the problem.
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.
Assignee: general → bbouvier
Status: NEW → ASSIGNED
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.
Attached patch Temporary fix (obsolete) — Splinter Review
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.
Attachment #803437 - Flags: review?(terrence)
Comment on attachment 803437 [details] [diff] [review]
Temporary fix

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

r=me
Attachment #803437 - Flags: review?(terrence) → review+
Attached patch Cleaner fixSplinter Review
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.
Attachment #803437 - Attachment is obsolete: true
Attachment #803460 - Flags: review?(terrence)
Alice, do you know if the lack of textures when zooming out is also due to the same cset?
Flags: needinfo?(alice0775)
Comment on attachment 803460 [details] [diff] [review]
Cleaner fix

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

Much nicer. r=me
Attachment #803460 - Flags: review?(terrence) → review+
Whiteboard: [leave open]
(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
Flags: needinfo?(alice0775)
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
Attachment #803432 - Flags: review?(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"
Attachment #803432 - Flags: review?(sstangl) → review+
Also added a comment for explaining why the check makes sense.

https://hg.mozilla.org/integration/mozilla-inbound/rev/13568a3576cd
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/13568a3576cd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 916816
Should this be considered fixed with Bug 916816 describing the exact same scenario?
No longer depends on: 916816
(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.
Depends on: 938431
You need to log in before you can comment on or make changes to this bug.