886230 - Assertion failure: "should have already reflowed the kid" with DRAWWINDOW_DO_NOT_FLUSH

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi 
2. Set
     user_pref("svg.text.css-frames.enabled", true);
3. Load the testcase

Assertion failure: !(((kid)->GetStateBits() & ((nsFrameState(1) << (10)) | (nsFrameState(1) << (12)))) != 0) (should have already reflowed the kid), at layout/svg/nsSVGTextFrame2.cpp:4837

This assertion was added in bug 885642.

DRAWWINDOW_DO_NOT_FLUSH is used by Firefox's thumbnail service, IIRC.  So there's probably a way to trigger this bug without the extension.

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Cameron McCormack (:heycam)]

I think the best we can do is just not paint the text, which is what we are doing now (but also asserting).  It'd be good to keep the assertion in there for other cases where we know we should have reflowed the text.  I wonder if there's a way to detect that we're in a DRAWWINDOW_DO_NOT_FLUSH?

Comment 3

[Cameron McCormack (:heycam)]

roc, would it be OK to store a bit (only #ifdef DEBUG) in PresShell::mRenderFlags while we are painting the window under DrawWindow(..., DRAWWINDOW_DO_NOT_FLUSH) that nsSVGTextFrame2::PaintSVG could then check to see whether it can skip the assertion that the text frames are not dirty?

Or otherwise, have a flag on the nsDisplayListBuilder that nsDisplaySVGText::Paint can look at?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #3)
> roc, would it be OK to store a bit (only #ifdef DEBUG) in
> PresShell::mRenderFlags while we are painting the window under
> DrawWindow(..., DRAWWINDOW_DO_NOT_FLUSH) that nsSVGTextFrame2::PaintSVG
> could then check to see whether it can skip the assertion that the text
> frames are not dirty?

Yes I think that's reasonable.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch

I wasn't sure that it was really worth the clutter of wrapping things in #ifdefs in the end.

Comment 6

[Cameron McCormack (:heycam)]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=d9eb13626402

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 767027 [details] [diff] [review]
patch

Review of attachment 767027 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1134,5 @@
> +      flags |= ctx.DRAWWINDOW_DRAW_CARET;
> +    }
> +    if (options.DRAWWINDOW_DO_NOT_FLUSH) {
> +      flags |= ctx.DRAWWINDOW_DO_NOT_FLUSH;
> +    }

I think you can write something like
  for (var option in options) {
    flags |= ctx[option];
  }
Hooray Javascript!

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> I think you can write something like
>   for (var option in options) {
>     flags |= ctx[option];
>   }
> Hooray Javascript!

Good one. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c237784ce9

Comment 9

[Ed Morley [:emorley]]

Backed out for B2G mochitest-9 failures in test_bug582181-1.html:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=727736b233b3&tochange=7e06e9a0a8d9&rev=d7c237784ce9&jobname=mochitest-9

Also intermittently on OS X:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24563041&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4441fe8e2ead

Comment 10

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c965b82024eb

I had to do:

  for (var option in options) {
    flags |= options[option] && ctx[option];
  }

to make { DRAWWINDOW_DO_NOT_FLUSH: false } not set that flag.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c965b82024eb