Bug 84242 – Web Inspector: Radio buttons in Audits tabs don't work

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

What steps will reproduce the problem?
1. Open any webpage in Chrome. (eg www.google.com).
2. Right click anywhere on it and select Inspect element.
3. Now click the Audits tab. Select the radio button "Reload Page and Audit on load"
[if the radiobuttons work for you, follow the next steps WITHOUT switching to another window (i.e. the entire step sequence must be run with the browser window always focused)]
4. Switch to the Profiles panel and back to the Audits window
5. Select the "Audits" tree element in the lefthand tree to open the audits launcher view. Now the radio buttons look dead.

What is the expected result?
The radio button should toggle the option we click another one.

What happens instead?
The radio button don't toggle.

(Upstreaming http://code.google.com/p/chromium/issues/detail?id=123289)

Now some investigation results:
1. The radio buttons do toggle but are never repainted. This can be checked by clicking "Audit Present State" and "Run" - the audits will run without reloading the page.
2. When steps 4 and 5 are run (I only very rarely can reproduce the issue in Chromium without them), certain container DIVs corresponding to removed views are detached from the DOM tree to speed up rendering, and those corresponding to shown panels are inserted (element.appendChild) - this is the common way to switch views in Web Inspector.
3. When a radio button is clicked, HTMLInputElement::setChecked() invokes setNeedsStyleRecalc(), which calls markAncestorsWithChildNeedsStyleRecalc(). Its loop stops quickly (like, at the parent <label> element, for which childNeedsStyleRecalc() == true), and I believe the algorithm presumes that this condition holds for all its ancestors. However, this seems wrong: if I comment out the "&& !p->childNeedsStyleRecalc()" part of the condition, the radio buttons start working as expected. Stepping through the loop (with this fix in place), I found out that p->childNeedsStyleRecalc() == false for the container DIV that was detached (element.removeChild) and then re-inserted (element.appendChild) into the Web Inspector DOM, as well as its ancestors chain.
4. I thought that it would be sufficient to add
  setChildNeedsStyleRecalc();
  markAncestorsWithChildNeedsStyleRecalc();
at the end of Node::attach() but it proved wrong - I observed the absence of attached view contents altogether.
5. I noticed that lazyAttach() runs markAncestorsWithChildNeedsStyleRecalc() but could not deduce what lazyAttach() is for, and when it comes into play instead of attach() (I saw some work by jamesr@ to retain lazyAttach() as the only option.)

Does anyone have any insight of what's happening/how this works and what can be done to fix this? (Obviously, not the "Web Inspector" component...)

[Simon Fraser (smfr)]

Do you have a regression range?

[Alexander Pavlov (apavlov)]

(In reply to comment #1)
> Do you have a regression range?

Unfortunately, not. While bisecting, I ended up with my own change that introduced the updated DOM layout which manifested the bug (around November 2011), which means the bug has been sitting there for a while...

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (In reply to comment #1)
> > Do you have a regression range?
> 
> Unfortunately, not. While bisecting, I ended up with my own change that introduced the updated DOM layout which manifested the bug (around November 2011), which means the bug has been sitting there for a while...

Hmm, I seem to have come up with a reduced test case, so will try to bisect now.

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Do you have a regression range?
> > 
> > Unfortunately, not. While bisecting, I ended up with my own change that introduced the updated DOM layout which manifested the bug (around November 2011), which means the bug has been sitting there for a while...
> 
> Hmm, I seem to have come up with a reduced test case, so will try to bisect now.

Got it. http://trac.webkit.org/changeset/83065

Addendum: Step 3 above MUST be followed by "Click the Run button", obviously.

I've got (an invisible) progress indicator to the right of the "Run" button, if that matters (and that should.) It shows up when the "Run" button is clicked, and gets hidden when the page has reloaded and audits have finished running.

[Hajime Morrita]

(In reply to comment #4)
> > 
> > Hmm, I seem to have come up with a reduced test case, so will try to bisect now.
> 
> Got it. http://trac.webkit.org/changeset/83065
> 
> Addendum: Step 3 above MUST be followed by "Click the Run button", obviously.
> 
> I've got (an invisible) progress indicator to the right of the "Run" button, if that matters (and that should.) It shows up when the "Run" button is clicked, and gets hidden when the page has reloaded and audits have finished running.

Hmm interesting. I'm interesting. Since I'm in travel (for the meeting),
I cannot take deeper look in a week. But I'll do after going back.

[Alexander Pavlov (apavlov)]

Presumably, a stackoverflow user is hitting a manifestation of the same bug: http://stackoverflow.com/questions/10892142/progress-bar-element-breaking-chrome-rendering-unless-dev-tools-open.

Hajime, could you take a look? The issue is easily reproducible in the DevTools.

[Alexander Pavlov (apavlov)]

Seemingly, the issue gets fixed by rewriting didElementStateChange() as follows:

void HTMLProgressElement::didElementStateChange()
{
    m_value->setWidthPercentage(position() * 100);
    if (renderer() && renderer()->isProgress()) {
        renderer()->updateFromElement();
        setNeedsStyleRecalc();
    }
}

Hajime, can you tell if this is the right way to fix this?

[Hajime Morrita]

(In reply to comment #7)
> Seemingly, the issue gets fixed by rewriting didElementStateChange() as follows:
> 
> void HTMLProgressElement::didElementStateChange()
> {
>     m_value->setWidthPercentage(position() * 100);
>     if (renderer() && renderer()->isProgress()) {
>         renderer()->updateFromElement();
>         setNeedsStyleRecalc();
>     }
> }
> 
> Hajime, can you tell if this is the right way to fix this?
Sorry for the slow response. Looks like a safe change.
http://code.google.com/p/chromium/issues/detail?id=132014 also indicates
we miss some style recalculation request.

[Alexander Pavlov (apavlov)]

> > Hajime, can you tell if this is the right way to fix this?
> Sorry for the slow response. Looks like a safe change.
> http://code.google.com/p/chromium/issues/detail?id=132014 also indicates
> we miss some style recalculation request.

Well, this did not fix http://code.google.com/p/chromium/issues/detail?id=132014 for me, so there's something else getting in the way...

[Alexander Pavlov (apavlov)]

Looks fixed as of r138555.