Bug 188032 – [WIN] Crash when trying to access store pages

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WIN] Regression(r213614): Crash when trying to access store pages

[Myles C. Maxfield]

Created attachment 345806 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/42606175>

[Brent Fulgham]

Comment on attachment 345806 [details]
Patch

R=me if the boys are happy.

[Brent Fulgham]

S/boys/bots/

[Myles C. Maxfield]

<rdar://problem/42467016>

[John N. Lehner]

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.

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[John N. Lehner]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Myles C. Maxfield]

Created attachment 345897 [details]
Patch

[Myles C. Maxfield]

Created attachment 345902 [details]
Patch

[Myles C. Maxfield]

Created attachment 345917 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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.