1140737 - weather.com does not render completely or becomes non-responsive.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary [:streetwolf52]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150307043006

Steps to reproduce:

Make sure you have the bad cset installed.

Regression range:

Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc86a4e16519

Bad:  https://hg.mozilla.org/integration/mozilla-inbound/rev/cb5ec48a3391

Use a new profile.

Go to www.weather.com


Actual results:

Site does not render completely, can become non-responsive and Fx must be closed, or very rarely Fx will crash.


Expected results:

Page should render as it did before the 'bad' cset.

Comment 1

[Gary [:streetwolf52]]

weather.com works fine in safe-mode.  I've ruled out any of my add-ons but I notice that my graphic options in safe-mode appear to be basic ones.  Here's a comparison between a new profile and running my live system in safe mode:

New Profile

Graphics
--------

Adapter Description: AMD Radeon HD 7900 Series
Adapter Drivers: aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM: 3072
ClearType Parameters: D [ Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 300 ] D [ Gamma: 2200 Pixel Structure: R ClearType Level: 50 Enhanced Contrast: 100 ]
Device ID: 0x6798
Direct2D Enabled: true
DirectWrite Enabled: true (6.3.9600.17415)
Driver Date: 11-20-2014
Driver Version: 14.501.1003.0
GPU #2 Active: false
GPU Accelerated Windows: 2/2 Direct3D 11 (OMTC)
Subsys ID: 00000000
Vendor ID: 0x1002
WebGL Renderer: Google Inc. -- ANGLE (AMD Radeon HD 7900 Series Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote: true
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0


My live system in safe-mode

Graphics
--------

Adapter Description: AMD Radeon HD 7900 Series
Adapter Drivers: aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM: 3072
ClearType Parameters: D [ Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 300 ] D [ Gamma: 2200 Pixel Structure: R ClearType Level: 50 Enhanced Contrast: 100 ]
Device ID: 0x6798
DirectWrite Enabled: true (6.3.9600.17415)
Driver Date: 11-20-2014
Driver Version: 14.501.1003.0
GPU #2 Active: false
GPU Accelerated Windows: 0/1 Basic (OMTC)
Subsys ID: 00000000
Vendor ID: 0x1002
WebGL Renderer: Google Inc. -- ANGLE (AMD Radeon HD 7900 Series Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote: true
AzureCanvasBackend: skia
AzureContentBackend: cairo
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0

Comment 2

[Gary [:streetwolf52]]

I also get this script message:

A script on this page may be busy, or it may have stopped responding. You can stop the script now, open the script in the debugger, or let the script continue.

Script: http://www.weather.com/sites/a…bCSKW0ivtyz3wMQLlR9HRfudjrU.js:186

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The set of changes between those changesets is pretty small, and mostly related to:
 * JS
 * Media
 * safe browsing

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dc86a4e16519&tochange=cb5ec48a3391

Does the problem go away if you flip the browser.safebrowsing.enabled preference?

Comment 4

[Gary [:streetwolf52]]

Flipping safebrowsing didn't work.

Comment 5

[Gary [:streetwolf52]]

I'm also getting some rendering problems at other sites.  One such site is https://signin.optimum.net/.  I went back to the last 'good' cset and it displayed fine.  Problem might be a Windows 8.1 Fx 64-bit problem.

Comment 6

[Gary [:streetwolf52]]

If I set 'javascript.options.baselinejit' to false everything works fine.  Found out safe-mode also sets this to false which is why safe-mode didn't have the problem.

Comment 7

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Website breakage

Bisect says:

The first bad revision is:
changeset:   232351:0712a3d4b79c
user:        Jason Orendorff <[e-mail]>
date:        Tue Feb 03 19:51:40 2015 -0600
summary:     Bug 1113369, part 4 - [[Set]] ObjectOpResult support. r=Waldo, r=bz in dom, r=dvander in js/ipc, r=bholley in js/xpconnect.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> If I set 'javascript.options.baselinejit' to false everything works fine.

In fact, things even work fine with 'javascript.options.baselinejit' set to true, if I set 'javascript.options.ion' to false.

Comment 9

[B.J. Herbison]

The problem is in today's nightly build. weather.com doesn't work with a clean profile (partial display and "a web page is causing Nightly to run slowly") but works correctly in safe mode.

Comment 10

[Liz Henry (:lizzard)]

Tracking for 39 since this is a recent regression.

Comment 11

[Jason Orendorff [:jorendorff]]

Likely caused by bug 1141154. Taking.

Comment 12

[Jason Orendorff [:jorendorff]]

OK, my new theory is that this is caused by bug 1141329. Both those bugs have fixes, try servering as soon as I can...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix Ion setter cache to reserve the register the value being set lives in as needed, so other things don't stomp on that register

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8576059 [details] [diff] [review]
Fix Ion setter cache to reserve the register the value being set lives in as needed, so other things don't stomp on that register

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

::: js/src/jit/IonCaches.cpp
@@ +2388,4 @@
>      RegisterSet regSet(RegisterSet::All());
>      regSet.take(AnyRegister(object));
> +    if (!value.constant())
> +        regSet.takeUnchecked(value.reg());

Unfortunately this won't work on x86 for the PropertyOp case I think. We have 7 registers available (all regs except esp), and take() all of them atm:

* object reg
* scratchReg
* 5 arg*Reg registers.

With this patch, we need at least 9 registers.

To fix this, we can add value back to the set once we're done with it (after we push it), then take the arg*Reg registers after that (right before we use them).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix Ion setter cache to reserve the register the value being set lives in as needed, so other things don't stomp on that register

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8576112 [details] [diff] [review]
Fix Ion setter cache to reserve the register the value being set lives in as needed, so other things don't stomp on that register

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

::: js/src/jit/IonCaches.cpp
@@ +2478,5 @@
> +        // OK, now we can grab our remaining registers and grab the pointer to
> +        // what we just pushed into one of them.
> +        Register argJSContextReg = regSet.takeGeneral();
> +        Register argVpReg        = regSet.takeGeneral();
> +        // XXXbz Could we just reuse object for argObjReg?

We could do this:

masm.Push(object);
masm.movePtr(StackPointer, object);

It might be a little confusing when we use "object" later on, because it's now a different value. To avoid that we can do:

Register argObjReg = object;

Fine with me, although we have enough registers now.

Comment 17

[Jan de Mooij [:jandem]]

Btw, with most PropertyOp setters gone, maybe we can easily tweak the signature so that it needs fewer registers? Ideally we'd remove these getters/setters but that seems more complicated...

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cff616142313

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cff616142313