721284 - GMail broken on Windows builds with JS PGO enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Lately, I've been having numerous issues with GMail. When I send a message, the inbox often goes blank and clicking on folders on the side doesn't do anything. I've also had it where messages don't open when I click them. The only way to get it to stop acting glitchy is to reload the page. I've confirmed that these problems occur on a clean profile.

The error console catches a few warnings after reloading (but none while it's being glitchy):
Timestamp: 1/25/2012 8:48:08 PM
Error: attempt to run compile-and-go script on a cleared scope
Source File: https://mail.google.com/mail/?ui=2&view=js&name=main,tlist&ver=GgkbWT1cpLY.en.&am=!K44dex2vU9fZQv2QwUQ3cOvs29WB0V7rOEb7Sqjjbx092iPTTWejs2AkSSLTvRoEbw
Line: 407

Timestamp: 1/25/2012 8:49:10 PM
Error: uncaught exception: CustomError: Error in protected function: too much recursion

I haven't nailed down a regression range yet.

Comment 1

[Andrew McCreight [:mccr8]]

Can you reproduce this in safe mode?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Works fine in safe mode. Given that my clean profile had no extensions, I'm going to assume this is JIT-related. I'm installing mozregression now. We'll see what it finds.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I can't reproduce with nightlies using mozregression. My personal builds use VC10 SP1 with JS PGO enabled. I'm going to do a tryserver push with JS PGO turned on to see if it reproduces with that build.

Comment 4

[Luke Wagner [:luke]]

As for the "cleared global" error, I've seen that consistently in google apps.

Comment 5

[David Mandelin [:dmandelin]]

Marking dep on ClearScope. That might not be the only problem though.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I did a Try run with JS PGO enabled and see the same issues. If someone wants to reproduce, try the build here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dfd45f64ac35/try-win32/

https://tbpl.mozilla.org/?tree=Try&rev=dfd45f64ac35

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I ran tests this time. Looks like PGO is in fact breaking something. Question is - compiler bug or "real" bug?
https://tbpl.mozilla.org/?tree=Try&rev=709b3fab47c0

In jsreftest:
BUGNUMBER: 380237
STATUS: Generator expressions - sudoku
optionsReset: caught [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: file:///c:/talos-slave/test/build/jsreftest/tests/browser.js :: options :: line 211"  data: no]
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8/genexps/regress-380237-01.js | No test results reported. (SCRIPT)

In xpcshell:
TEST-INFO | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell\test_general.js | running test ...

command timed out: 1200 seconds without output, attempting to kill
SIGKILL failed to kill process
using fake rc=-1
program finished with exit code -1

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Bisecting with hg revert (thank God for Try), I've narrowed down the regressing patch to bug 706914. With any luck, the same issue leading to comment 7's failures is the cause of the GMail issues (seeing as how they appear to go hand in hand).

Brian, I know that JS PGO is not really a supported build config, but there could very well be an actual issue here that PGO just happens to be poking in the right manner. It's worth noting that the same issue appears with both VC8 and VC10. I really hope that the time I've spent narrowing this down hasn't been a total waste.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

At Brian's request, I attempted a build changing CHUNK_LIMIT in methodjit/Compiler.cpp to 'uint32_t(-1)'. The test failures still reproduce and GMail still doesn't work properly.

https://tbpl.mozilla.org/?tree=Try&rev=b864c1ec4c1d

Comment 10

[Brian Hackett [Laid off!]]

With the change above there shouldn't be any difference in generated jitcode vs. an earlier revision, and the problem is probably a PGO-related miscompilation somewhere in the C++ changes.  Does anyone know if there is a way to selectively disable PGO at a compilation unit (or finer) granularity?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Looking at http://msdn.microsoft.com/en-us/library/ms235601.aspx, it appears that |#pragma optimize("", off)| might work on a function by function basis.

Comment 12

[Andrew McCreight [:mccr8]]

Yes, it looks like that was used in bug 563318 to disable optimization of a particular problematic function.

Comment 13

[Makoto Kato [:m_kato]]

Attachment: pgo workaround

Comment 14

[xunxun]

(In reply to Makoto Kato from comment #13)
> Created attachment 627415 [details] [diff] [review]
> pgo workaround

I remembered VC2010 use -fp:precise by default.

Some functions around use #pragma optimize("g", off) and #pragma optimize("g", off) maybe better than using -GL-.

Comment 15

[xunxun]

(In reply to Makoto Kato from comment #13)
> Created attachment 627415 [details] [diff] [review]
> pgo workaround

I remembered VC2010 use -fp:precise by default.

Some functions around use #pragma optimize("g", off) and #pragma optimize("g", off) maybe better than using -GL-.(In reply to xunxun from comment #14)
> (In reply to Makoto Kato from comment #13)
> > Created attachment 627415 [details] [diff] [review]
> > pgo workaround
> 
> I remembered VC2010 use -fp:precise by default.
> 
> Some functions around use #pragma optimize("g", off) and #pragma
> optimize("g", off) maybe better than using -GL-.

use #pragma optimize("g", off) and #pragma optimize("g", off)

-->

use #pragma optimize("g", off) and #pragma optimize("g", on)

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 627415 [details] [diff] [review]
pgo workaround

Passed on try server.
https://tbpl.mozilla.org/?tree=Try&rev=0e817439c08f

I think that some files should be except to PGO optimization due to x86 compiler issue. Also this workaround isn't needed on x64 compiler.
To be safety, use -GL- instead of #pragma.

And you know, since Win32 build uses mozjs.dll, I believe that no effect for xul.dll linking memory issue.

Comment 17

[David Mandelin [:dmandelin]]

Comment on attachment 627415 [details] [diff] [review]
pgo workaround

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

Seems reasonable. Thanks for looking into this--really tricky stuff.

Comment 18

[Makoto Kato [:m_kato]]

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

Comment 19

[Ed Morley [:emorley]]

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