Bug 69412 – REGRESSION (r95399): Web process hangs when opening documents on Google Docs

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Juan Falgueras]

Entering in Google Doc (new interface at least) with Nightly Build hangs it.

[Alexey Proskuryakov]

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?

[Juan Falgueras]

Created attachment 109943 [details]
Safari WebContent sample

[Juan Falgueras]

Created attachment 109944 [details]
Webkit sample

[Juan Falgueras]

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.

[Alexey Proskuryakov]

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>.

[Filip Pizlo]

(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!

[Filip Pizlo]

*** Bug 69775 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 110392 [details]
the patch

[WebKit Review Bot]

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.

[Oliver Hunt]

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)) ?

[Filip Pizlo]

> >> 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.

[Filip Pizlo]

Created attachment 110399 [details]
the patch

[Oliver Hunt]

Comment on attachment 110399 [details]
the patch

Have you verified that 32_64 is correct?

[Filip Pizlo]

(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...

[Filip Pizlo]

Created attachment 110412 [details]
the patch

added 32_64 changes

[Adam Barth]

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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/97099