Bug 188032

Created attachment 345806 [details]
Patch

<rdar://problem/42606175>

Comment on attachment 345806 [details]
Patch

R=me if the boys are happy.

S/boys/bots/

<rdar://problem/42467016>

This won't fix rdar://problem/42467016; WebKit!WebCore::FontCascade::widthOfTextRange() is not called prior to the overflow in WebCore::GlyphBuffer::offsetAt().

Because WebCore::DisplayList::Replayer::replay() is on the stack at the time of overflow I compared WebCore::TextPainter::paintTextOrEmphasisMarks() in our previous release with today and 

 m_context.drawEmphasisMarks(font, textRun, emphasisMark, textOrigin + FloatSize(0, emphasisMarkOffset), startOffset, endOffset);

was changed to

// Replaying back a whole cached glyph run to the GraphicsContext.
m_context.translate(textOrigin);
DisplayList::Replayer replayer(m_context, *m_glyphDisplayList);
replayer.replay();
m_context.translate(-textOrigin);

If I patch out the DisplayList::Replayer::replay() call with a 5-byte NOP, the crash goes away.

(In reply to John N. Lehner from comment #6)
> This won't fix rdar://problem/42467016;
> WebKit!WebCore::FontCascade::widthOfTextRange() is not called prior to the
> overflow in WebCore::GlyphBuffer::offsetAt().
> 
> Because WebCore::DisplayList::Replayer::replay() is on the stack at the time
> of overflow I compared WebCore::TextPainter::paintTextOrEmphasisMarks() in
> our previous release with today and 
> 
>  m_context.drawEmphasisMarks(font, textRun, emphasisMark, textOrigin +
> FloatSize(0, emphasisMarkOffset), startOffset, endOffset);
> 
> was changed to
> 
> // Replaying back a whole cached glyph run to the GraphicsContext.
> m_context.translate(textOrigin);
> DisplayList::Replayer replayer(m_context, *m_glyphDisplayList);
> replayer.replay();
> m_context.translate(-textOrigin);
> 
> If I patch out the DisplayList::Replayer::replay() call with a 5-byte NOP,
> the crash goes away.

Ah, okay! That definitely helps.

Looks like https://bugs.webkit.org/show_bug.cgi?id=178750 enabled display list usage without an easy way to disable it on Windows

To clarify for open source, the generateGlyphBuffer() call in WebCore::DisplayList::DrawGlyphs::apply() uses the GlyphBufferAdvance overload of GlyphBuffer::add() (!USE(WINGDI)) and that doesn't update m_offsets like 

void add(Glyph glyph, const Font* font, float width, unsigned offsetInString = noOffset, const FloatSize* offset = 0)

does, so WebCore::GlyphBuffer::offsetAt() generates the CrashOnOverflow

(In reply to Myles C. Maxfield from comment #8)
> Looks like https://bugs.webkit.org/show_bug.cgi?id=178750 enabled display
> list usage without an easy way to disable it on Windows

Fixing display lists to work on Windows requires either getting rid of the offsets member of GlyphBuffer or recording all the offsets in the DrawGlyphs object.

Tomorrow I'll get rid of the offsets member of GlyphBuffer.

Comment on attachment 345806 [details]
Patch

Attachment 345806 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8661025

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

Created attachment 345838 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Created attachment 345897 [details]
Patch

Created attachment 345902 [details]
Patch

Created attachment 345917 [details]
Patch

Comment on attachment 345917 [details]
Patch

Attachment 345917 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8673844

New failing tests:
fast/text/initial-advance-in-intermediate-run-complex.html
fast/text/complex-first-glyph-with-initial-advance.html

Created attachment 345930 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Landing despite test failures because this will fix the crash; looking into the failures today.

Committed r234318: <https://trac.webkit.org/changeset/234318>

(In reply to Myles C. Maxfield from comment #20)
> Committed r234318: <https://trac.webkit.org/changeset/234318>

This appears to have caused bug 188168.

(In reply to mitz from comment #21)
> (In reply to Myles C. Maxfield from comment #20)
> > Committed r234318: <https://trac.webkit.org/changeset/234318>
> 
> This appears to have caused bug 188168.

Oh, I guess EWS has warned of this in comment 17.