[Chromium][Win] Crashes with <keygen> with huge padding.

[Hajime Morrita]

Reported at http://code.google.com/p/chromium/issues/detail?id=47050 .
Attached file makes Windows Chromium crash.

[Hajime Morrita]

Created attachment 60681 [details]
a repro

[Hajime Morrita]

Created attachment 60690 [details]
patch v0

[Adam Barth]

Comment on attachment 60690 [details]
patch v0

The general form of this patch looks good.  I like the test, and I like compositing over inheritance.  I just don't quite understand enough of how things work in the rendering folder to R+ this patch.  Hopefully this will be an easy review for some of our rendering friends.

[David Levin]

Comment on attachment 60690 [details]
patch v0

WebCore/rendering/RenderThemeChromiumWin.cpp:91
 +          PassOwnPtr<TransparencyWin> fallback = adoptPtr(new TransparencyWin());
Style nit: Use OwnPtr. PassOwnPtr should only be used for (return and input) arguments.

WebCore/rendering/RenderThemeChromiumWin.cpp:89
 +          // Because TransparencyWin doesn't allocate temporally buffer for NoLayer,
s/temporally/temporary/

WebCore/ChangeLog:9
 +          allocate a temporal buffer to composition.  This change add
s/add/adds a/

WebCore/ChangeLog:10
 +          fallback path to ThemePainter to handle the bufer allocation
s/bufer/buffer/

WebCore/ChangeLog:13
 +          ThemePainter is no lonnger a subclass of TransparencyWin.  It has
s/lonnger/longer/

WebCore/ChangeLog:9
 +          allocate a temporal buffer to composition.  This change add
s/to/for/

WebCore/ChangeLog:19
 +          (WebCore::TransparencyWin::destContext): Added to expose an internal state.
Exposed state for use in the creation of the fallback window.

WebCore/ChangeLog:22
 +          (WebCore::TransparencyWin::transformMode): Added to expose an internal state.
s/Added to expose an internal state./Ditto./


WebCore/ChangeLog:24
 +          (WebCore::ThemePainter): Added a fallabck path.
s/fallabck/fallback/

WebCore/rendering/RenderThemeChromiumWin.cpp:92
 +          fallback->init(m_helper.destContext(), TransparencyWin::NoLayer, m_helper.transformMode(), m_helper.sourceRect());
ASSERT(fallback->context());
?


WebCore/rendering/RenderThemeChromiumWin.cpp:88
 +          ASSERT(!m_fallbackHelper);
This assert seems odd since this method doesn't even touch m_fallbackHelper.

WebCore/rendering/RenderThemeChromiumWin.cpp:90
 +          // it can be used for fallback when buffer allocation failed.
Why is it ok to use NoLayer?

WebCore/platform/graphics/chromium/TransparencyWin.h:148
 +      GraphicsContext* destContext() const { return m_destContext; }
Avoid abbreviations: s/dest/destination/ (for the method name).

WebCore/rendering/RenderThemeChromiumWin.cpp:65
 +          m_helper.init(context, getLayerMode(context, transformMode), transformMode, r);
Why not just do this?

if (!m_helper.context())
    m_helper(context, TransparencyWin::NoLayer, transformMode, r);

Then, get rid of a lot of stuff like helper(), etc.

[Hajime Morrita]

Created attachment 61477 [details]
patch v1; took feedbacks

[Brett Wilson (Google)]

This Looks Good To Me.

[Hajime Morrita]

> This Looks Good To Me.
Thanks! I'll land this.

[Hajime Morrita]

Created attachment 61595 [details]
patch to land

[WebKit Commit Bot]

Comment on attachment 61595 [details]
patch to land

Rejecting patch 61595 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 61595, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
1595&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=41737&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 61595 from bug 41737.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Hajime Morrita]

Created attachment 61606 [details]
patch to land (retry)

[WebKit Commit Bot]

Comment on attachment 61606 [details]
patch to land (retry)

Rejecting patch 61606 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20689 test cases.
fast/forms/large-parts.html -> failed

Exiting early after 1 failures. 7632 tests run.
162.44s total testing time

7631 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://webkit-commit-queue.appspot.com/results/3409408

[Hajime Morrita]

Created attachment 61761 [details]
last patch was broken during a patch juggling. fixed and retrying

[WebKit Commit Bot]

Comment on attachment 61761 [details]
last patch was broken during a patch juggling. fixed and retrying

Clearing flags on attachment: 61761

Committed r63511: <http://trac.webkit.org/changeset/63511>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.