Bug 69412 - REGRESSION (r95399): Web process hangs when opening documents on Google Docs
Summary: REGRESSION (r95399): Web process hangs when opening documents on Google Docs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Major
Assignee: Nobody
URL: http://docs.google.com
Keywords: Regression
: 69775 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-10-05 04:55 PDT by Juan Falgueras
Modified: 2011-10-20 13:32 PDT (History)
6 users (show)

See Also:


Attachments
Safari WebContent sample (93.30 KB, text/plain)
2011-10-06 04:12 PDT, Juan Falgueras
no flags Details
Webkit sample (34.20 KB, text/plain)
2011-10-06 04:13 PDT, Juan Falgueras
no flags Details
the patch (7.47 KB, patch)
2011-10-10 13:36 PDT, Filip Pizlo
oliver: review-
Details | Formatted Diff | Diff
the patch (6.44 KB, patch)
2011-10-10 13:51 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (7.58 KB, patch)
2011-10-10 15:14 PDT, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Juan Falgueras 2011-10-05 04:55:05 PDT
Entering in Google Doc (new interface at least) with Nightly Build hangs it.
Comment 1 Alexey Proskuryakov 2011-10-05 23:38:53 PDT
I can open docs.google.com with r96636 without problem.

What nightly build are you seeing this with? Could you please post more detailed steps to reproduce?

Also could you please run "sample Safari" and "sample WebProcess" in command line, and attach the results if you can still reproduce this?
Comment 2 Juan Falgueras 2011-10-06 04:12:52 PDT
Created attachment 109943 [details]
Safari WebContent sample
Comment 3 Juan Falgueras 2011-10-06 04:13:25 PDT
Created attachment 109944 [details]
Webkit sample
Comment 4 Juan Falgueras 2011-10-06 04:13:38 PDT
The problem is not opening the Site, but a simple Document, My KBuild: r96792

If you get into any Doc it draws only its frame, but the contents (JavaScript related?) makes Knightly B. hang, irresponsive. After that (you must close the Gdoc via clicking the window close button) Webkit tells you you need to reload every page.


Juan F.
Comment 5 Alexey Proskuryakov 2011-10-06 11:02:30 PDT
Thank you!

This doesn't happen with every document, but I can now reproduce with <https://docs.google.com/document/d/1as5xYjyMSCph4960iz0-Kb7hZKf_L6f2vts57NMcVBI/edit?hl=en_US> (no login required).

Regressed in <http://trac.webkit.org/changeset/95399>.
Comment 6 Filip Pizlo 2011-10-06 12:34:50 PDT
(In reply to comment #5)
> Thank you!
> 
> This doesn't happen with every document, but I can now reproduce with <https://docs.google.com/document/d/1as5xYjyMSCph4960iz0-Kb7hZKf_L6f2vts57NMcVBI/edit?hl=en_US> (no login required).
> 
> Regressed in <http://trac.webkit.org/changeset/95399>.

Thanks for zeroing in on this.  I'll take a look!
Comment 7 Filip Pizlo 2011-10-10 13:07:20 PDT
*** Bug 69775 has been marked as a duplicate of this bug. ***
Comment 8 Filip Pizlo 2011-10-10 13:21:49 PDT
OK, looks like I see what's going on.  The double case of ArithMin and ArithMax are totally broken.  When the two values are equal, it adds them together.  So Math.min(1.5, 1.5) == 3.

The reason for the adding is to have efficient support for Math.min(1.5, NaN) and Math.min(NaN, 1.5).  When the two values are unordered (either or both are NaN), Math.min() should return NaN.  Easiest way to do that is if you know one is NaN (but you don't know *which* one) then you add the two numbers together, which is gauranteed to result in NaN.

But the unordered case was erroneously being reached when the two values were equal.

Patch forthcoming.
Comment 9 Filip Pizlo 2011-10-10 13:36:18 PDT
Created attachment 110392 [details]
the patch
Comment 10 WebKit Review Bot 2011-10-10 13:38:30 PDT
Attachment 110392 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:688:  More than one command on the same line in if  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:707:  More than one command on the same line in if  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:711:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Oliver Hunt 2011-10-10 13:43:49 PDT
Comment on attachment 110392 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110392&action=review

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:688
>> +        if (false) return false;
> 
> More than one command on the same line in if  [whitespace/parens] [4]

This seems unlikely to be correct...

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:707
>> +        if (false) return false;
> 
> More than one command on the same line in if  [whitespace/parens] [4]

:D

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:711
>> +        if (false) return false;
> 
> More than one command on the same line in if  [whitespace/parens] [4]

:D

> Source/JavaScriptCore/jit/JIT.cpp:540
> -#if ENABLE(DFG_JIT)
> -    if (m_canBeOptimized)
> -        m_startOfCode = label();
> +#if ENABLE(DFG_JIT) || ENABLE(JIT_VERBOSE)
> +    m_startOfCode = label();

Shouldn't this be if (m_canBeOptimized || ENABLE(JIT_VERBOSE)) ?
Comment 12 Filip Pizlo 2011-10-10 13:49:49 PDT
> >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:688
> >> +        if (false) return false;
> > 
> > More than one command on the same line in if  [whitespace/parens] [4]
> 
> This seems unlikely to be correct…

Ahhhh, that's embarrassing.

> > Source/JavaScriptCore/jit/JIT.cpp:540
> > -#if ENABLE(DFG_JIT)
> > -    if (m_canBeOptimized)
> > -        m_startOfCode = label();
> > +#if ENABLE(DFG_JIT) || ENABLE(JIT_VERBOSE)
> > +    m_startOfCode = label();
> 
> Shouldn't this be if (m_canBeOptimized || ENABLE(JIT_VERBOSE)) ?

The alternative scares me, and label() is free.  I'm setting it whenever the field exists because otherwise you'd have something like:

#if ENABLE(DFG_JIT) || ENABLE(JIT_VERBOSE)
#if ENABLE(VALUE_PROFILER)
     if (m_canBeOptimized || ENABLE(JIT_VERBOSE))
#else
     if (true)
#endif
         m_startOfCode = label();
#endif

Or something like that.  Problem is that m_canBeOptimized is a field that doesn't always exist, and m_startOfCode is also a field that doesn't always exist.  I'd rather keep it simple.
Comment 13 Filip Pizlo 2011-10-10 13:51:42 PDT
Created attachment 110399 [details]
the patch
Comment 14 Oliver Hunt 2011-10-10 14:17:00 PDT
Comment on attachment 110399 [details]
the patch

Have you verified that 32_64 is correct?
Comment 15 Filip Pizlo 2011-10-10 14:19:29 PDT
(In reply to comment #14)
> (From update of attachment 110399 [details])
> Have you verified that 32_64 is correct?

Of course not, because that would have been the sensible thing to do.  New patch on the way...
Comment 16 Filip Pizlo 2011-10-10 15:14:41 PDT
Created attachment 110412 [details]
the patch

added 32_64 changes
Comment 17 Adam Barth 2011-10-15 01:50:04 PDT
Comment on attachment 110399 [details]
the patch

Cleared Oliver Hunt's review+ from obsolete attachment 110399 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 18 Filip Pizlo 2011-10-20 13:32:19 PDT
Landed in http://trac.webkit.org/changeset/97099