1388230 - Assertion failure: !color.IsEmpty() (Content node's GetValue() should return a valid color string (the default color, in case no valid color is set)),

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mc-debug rev 20170807-65507616792c.

Assertion failure: !color.IsEmpty() (Content node's GetValue() should return a valid color string (the default color, in case no valid color is set)), at /home/worker/workspace/build/src/layout/forms/nsColorControlFrame.cpp:100
#01: nsColorControlFrame::CreateAnonymousContent at layout/forms/nsColorControlFrame.cpp:72
#02: nsCSSFrameConstructor::GetAnonymousContent at layout/base/nsCSSFrameConstructor.cpp:4320
#03: nsCSSFrameConstructor::ProcessChildren at layout/base/nsCSSFrameConstructor.cpp:11232
#04: nsCSSFrameConstructor::ConstructFrameFromItemInternal at layout/base/nsCSSFrameConstructor.cpp:4177
#05: nsCSSFrameConstructor::ConstructFramesFromItem at layout/base/nsCSSFrameConstructor.cpp:6373
#06: nsCSSFrameConstructor::ConstructFramesFromItemList at layout/base/nsCSSFrameConstructor.cpp:11005
#07: nsCSSFrameConstructor::ContentAppended at layout/base/nsCSSFrameConstructor.cpp:7831
#08: mozilla::PresShell::ContentAppended at layout/base/PresShell.cpp:4439
#09: nsNodeUtils::ContentAppended at dom/base/nsNodeUtils.cpp:167
#10: nsHtml5TreeOperation::Append at parser/html/nsHtml5TreeOperation.cpp:185
#11: nsHtml5TreeOperation::Perform at parser/html/nsHtml5TreeOperation.cpp:627
#12: nsHtml5TreeOpExecutor::RunFlushLoop at parser/html/nsHtml5TreeOpExecutor.cpp:462
#13: nsHtml5ExecutorFlusher::Run at parser/html/nsHtml5StreamParser.cpp:131

Comment 1

[Ryan VanderMeulen [:RyanVM]]

I'm unable to bisect when this started, but it went away when bug 1348073 landed. I'm assuming that means the underlying defect might still be there, but are we likely to investigate this any further or can we just land the testcase as a crashtest and move on?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> it went away when bug 1348073
> landed. I'm assuming that means the underlying defect might still be there

Nice!  But, yes -- agreed -- it's likely there's still an issue here.

> but are we likely to investigate this any further or can we just land the
> testcase as a crashtest and move on?

I'll poke around in GDB today (with hg revision from comment 0), to see if I can figure out why the assert is failing & whether its condition needs adjusting (or perhaps if it points to a real & still-around issue).

Comment 3

[Daniel Holbert [:dholbert]]

OK, so what's *supposed* to happen here is:
   * When the <input type="color"> is created (or when its type is set), we call SetValueInternal() to set the empty string as the value.
   * And inside SetValueInternal, we normally call SanitizeValue, which promotes the empty string (or any unrecognized color) with #000000:
          if (mDoneCreating) {
            SanitizeValue(value);
          }

BUT, in the case where this assertion fails (in an old build), "mDoneCreating" was false when we traversed that code. And in fact, it's *still* false when we hit the assertion failure (which is inside of a call to nsHtml5TreeOperation::Append).

If I bypass the assertion, then we proceed and set mDoneCreating true slightly later (and we sanitize the color-string) inside of a call to nsHtml5TreeOperation::DoneCreatingElement.  So it all works out eventually.  So the problem here (to the extent that it's a problem) is that we're flushing frame-construction *while* the element is being created, and so some layout code sees a not-quite-finalized HTMLInputElement object (with an empty string as the value).

I think we should just relax the assertion and allow the string to be empty if NS_FRAME_FIRST_REFLOW is set (i.e. if we've just created this frame and haven't ever reflowed it).  That should cover this sort of scenario while still letting us assert that we get a nonempty string for other edge conditions (e.g. later on, if input.value gets cleared or 'type' gets dynamically adjusted, etc).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1388230: Make nsColorControlFrame::UpdateColor() a no-op if value is empty (which implies our element is still being appended).

nsColorControlFrame::UpdateColor() looks up the color value from the
corresponding <input> element -- and it expects to receive a valid color
string, regardless of what the user/author has done (or whether they've done
anything), thanks to the HTMLInputElement sanitization code that gets run when
the value is set.

As a basic sanity-check, UpdateColor() has an assertion to verify that the
value it receives is non-empty. However, if it happens to be called while the
element is still being appended (e.g. due to greedy frame construction), then
it *can* legitimately get an empty value.  So, the assertion isn't entirely valid!

Hence, this patch relaxes the assertion to only take effect after the frame has
been reflowed, and it also and makes UpdateColor() a no-op in that case.  This
is fine because we can expect that UpdateColor() will be called again (and will
see a non-empty color value at that point) before the frame gets
reflowed/painted.

Review commit: https://reviewboard.mozilla.org/r/186188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186188/

Comment 5

[Daniel Holbert [:dholbert]]

I verified that I can reproduce this bug on current trunk (and the attached crashtest fails) if I revert mozilla-central changeset 6b27a9f18281 (re-enabling greedy frame construction for editable regions).

And the bug goes away (the crashtest passes) if I take the attached patch on top of that.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 8914936 [details]
Bug 1388230: Make nsColorControlFrame::UpdateColor() a no-op if value is empty (which implies our element is still being appended).

https://reviewboard.mozilla.org/r/186188/#review191220

Absolutely fantastic commit message and commenting.

::: commit-message-8e840:15
(Diff revision 1)
> +value it receives is non-empty. However, if it happens to be called while the
> +element is still being appended (e.g. due to greedy frame construction), then
> +it *can* legitimately get an empty value.  So, the assertion isn't entirely valid!
> +
> +Hence, this patch relaxes the assertion to only take effect after the frame has
> +been reflowed, and it also and makes UpdateColor() a no-op in that case.  This

Extraneous "and".

::: layout/forms/nsColorControlFrame.cpp:117
(Diff revision 1)
> +  }
>  
> -  // Set the background-color style property of the swatch element to this color
> +  // Set the background-color CSS property of the swatch element to this color.
>    return mColorContent->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
> -      NS_LITERAL_STRING("background-color:") + color, true);
> +                                NS_LITERAL_STRING("background-color:") + color,
> +                                true);

I wouldn't be adverse to you putting in a /* aNotify */ comment here. ;)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8914936 [details]
Bug 1388230: Make nsColorControlFrame::UpdateColor() a no-op if value is empty (which implies our element is still being appended).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186188/diff/1-2/

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8914936 [details]
Bug 1388230: Make nsColorControlFrame::UpdateColor() a no-op if value is empty (which implies our element is still being appended).

https://reviewboard.mozilla.org/r/186188/#review191220

Thanks! Also, FWIW, in this latest revision, I've added the (simpler) crashtest from duplicate bug 1315408.

> Extraneous "and".

Good catch - fixed.

> I wouldn't be adverse to you putting in a /* aNotify */ comment here. ;)

Done.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8914936 [details]
Bug 1388230: Make nsColorControlFrame::UpdateColor() a no-op if value is empty (which implies our element is still being appended).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186188/diff/2-3/

Comment 11

[Daniel Holbert [:dholbert]]

(I just pushed one more update to add a newline at the end of the first crashtest, to avoid "\ No newline at end of file" in the raw patch.)

Try run (from before that newline tweak but it doesn't matter):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6dad9214fc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9051013eadf
Make nsColorControlFrame::UpdateColor() a no-op if value is empty (which implies our element is still being appended). r=jwatt

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Daniel Holbert [:dholbert]]

jet and I decided to set status-firefox57 to "unaffected" rather than "fixed", because:
 - the patch didn't land there [so not "fixed"]
 - still, the issue doesn't really exist on that release [not in a way that we now how to make manifest], because bug 1348073's patch avoids the problem, at least for this testcase.

Not sure we have a good status to describe that scenario; but it probably doesn't really matter much anyway.