HTMLInputElement::DoneCreatingElement() passes the wrong third argument to DoSetChecked() resulting in some unneeded work

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

See the code here: https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#6549

The third argument should be false, I think, since this call isn't changing the value.  Note that passing true here causes a DoSetCheckedChanged(true, false) here [1], but this code wants DoSetCheckedChanged(false, false) [2].

Now all of this bites us because of the underlying UpdateState() call here which can be expensive and shows up in Speedometer profiles: http://bit.ly/2t1DjC8.  This is a focused view as an example: http://bit.ly/2t1gPBg

The simple fix is as green as try can be these days: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e555fa8b7e8af5845bdc3129157d445380c4078

[1] https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#3231
[2] https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#6550
Also this is a practical example on why boolean arguments are a bad idea.  :-(  We should really fix this code, but it's late, and I'm too tired to write the bigger patch...
Comment on attachment 8880693 [details] [diff] [review]
Pass the correct argument to DoSetChecked() in HTMLInputElement::DoneCreatingElement() to avoid doing excessive work

So...  We have two relevant codepaths here.  There's DoneCreatingElement, which does:

    DoSetChecked(DefaultChecked(), false, true);
    DoSetCheckedChanged(false, false);

(the arg ordering is a bit annoying here; DoSetChecked takes "checked", "notify", "setvaluechanged", while DoSetCheckedChanged takes "checkedchanged", "notify).  DoSetChecked in this case calls DoSetCheckedChanged(true, false).  This is the call you're proposing we optimize out.

The other relevant codepath is AfterSetAttr for the "checked" attribute, if it happens after DoneCreatingElement.  That does:

    DoSetChecked(DefaultChecked(), true, true);
    SetCheckedChanged(false);

which is the same exact thing, but with aNotify == true (SetCheckedChanged(arg) just calls DoSetCheckedChanged(arg, true)).  We should certainly keep these codepaths in sync with each other, at the very least.

OK, so why are we doing this bizarre "DoSetCheckedChanged(true, aNotify)" followed by "DoSetCheckedChanged(false, aNotify)"?  For most controls that's an expensive no-op.  For the case of radio controls.... I guess it's still a no-op, but even more expensive?

Anyway, given that I think we should:

1)  Pass false for the third arg in both places (DoneCreatingElement and AfterSetAttr) so attr setting is consistent whether it happens before or after DoneCreatingElement.

2)  Remove the DoSetCheckedChanged(false, aNotify) calls, since they will now be (not too expensive) no-ops, right?
Attachment #8880693 - Flags: review?(bzbarsky) → review-
Attachment #8880693 - Attachment is obsolete: true
Comment on attachment 8881016 [details] [diff] [review]
Pass the correct argument to DoSetChecked() in HTMLInputElement::DoneCreatingElement() and AfterSetAttr() and delete the no-op DoSetCheckedChanged() call there to avoid doing excessive work

r=me
Attachment #8881016 - Flags: review?(bzbarsky) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c215e7162d
Pass the correct argument to DoSetChecked() in HTMLInputElement::DoneCreatingElement() and AfterSetAttr() and delete the no-op DoSetCheckedChanged() call there to avoid doing excessive work; r=bzbarsky
https://hg.mozilla.org/mozilla-central/rev/c3c215e7162d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.