Closed Bug 1388230 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file 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
Flags: in-testsuite?
Priority: -- → P3
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?
(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).
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).
Assignee: nobody → dholbert
Depends on: 875275
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.
Flags: needinfo?(dholbert)
Duplicate of this bug: 1315408
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. ;)
Attachment #8914936 - Flags: review?(jwatt) → review+
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.
(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
Pushed by dholbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c9051013eadf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
You need to log in before you can comment on or make changes to this bug.