Bug 105002 – trailing whitespace sometimes goes missing when inserted via innerHTML

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: WebKit Misc.

when running fast/workers/shared-worker-location.html in content_shell, it flakily fails because log messages that have a trailing whitespace actually don't have it. The code

document.getElementById("result").innerHTML += message + "<br>";  (from resources/worker-location.html)

sometimes skips the trailing whitespace of "message". This seems to happen esp. when the test was running very fast. E.g. instead of

location.host:_   (where _ is a whitespace), the test result is location.host:

This reproduces pretty well when running the test in content_shell on Linux, and the patch from https://codereview.chromium.org/11576003/ applied. The patch disables graphical output which speeds up content_shell a lot.

[Eric Seidel (no email)]

I see, so you're making it so we don't paint in testshell?  Are we sure we're still laying out?  whitespace is resolved in the linebox tree iirc.

(In reply to comment #1)
> I see, so you're making it so we don't paint in testshell?  Are we sure we're still laying out?  whitespace is resolved in the linebox tree iirc.

Yes, I'm not relying on the content's module backing store for generating the dumps, but replicate the behavior of DRT (i.e. layout and dump at the end of the test only).

Note that this occurs without the patch as well, just not as consistent.

[Eric Seidel (no email)]

http://trac.webkit.org/browser/trunk/LayoutTests/fast/workers/resources/worker-location.js
is the JS in question.

I would guess that notifyDone is not necessarily waiting on layout() to have happened before dumping.  That seems strange though, as I would expect it to.

[Eric Seidel (no email)]

The flakiness dashboard appears to be down?  Otherwise I'd post a link here as it would be nice to see the failures with my own eyes. :)

I don't believe this is parser related, as the parser doesn't collapse whitespace, it just ignores certain illegal characters and otherswise constructs the DOM from the original source. :)

(In reply to comment #4)
> The flakiness dashboard appears to be down?  Otherwise I'd post a link here as it would be nice to see the failures with my own eyes. :)

The test is skipped on chromium because DRT doesn't support shared workers

> 
> I don't believe this is parser related, as the parser doesn't collapse whitespace, it just ignores certain illegal characters and otherswise constructs the DOM from the original source. :)

Note that DRT only invokes layout() when generating pixel results.

I can try to invoke layout() before getting document.body.innerText (which is what DRT does to get text results) and see if that changes the outcome

[Eric Seidel (no email)]

Jochen says DRT uses innerTExt to dump the text content (I'd like to see the code, but I believe him).

String Element::innerText()
{
    // We need to update layout, since plainText uses line boxes in the render tree.
    document()->updateLayoutIgnorePendingStylesheets();

    if (!renderer())
        return textContent(true);

    return plainText(rangeOfContents(const_cast<Element*>(this)).get());
}

it's possible that the flakiness comes from having a render or not.

(In reply to comment #6)
> Jochen says DRT uses innerTExt to dump the text content (I'd like to see the code, but I believe him).

It's here: http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/TestShell.cpp#L408

> 
> String Element::innerText()
> {
>     // We need to update layout, since plainText uses line boxes in the render tree.
>     document()->updateLayoutIgnorePendingStylesheets();
> 
>     if (!renderer())
>         return textContent(true);
> 
>     return plainText(rangeOfContents(const_cast<Element*>(this)).get());
> }
> 
> it's possible that the flakiness comes from having a render or not.

is there a way to force having a renderer()?

[Eric Seidel (no email)]

(In reply to comment #7)
> (In reply to comment #6)
> > it's possible that the flakiness comes from having a render or not.
> 
> is there a way to force having a renderer()?

Elements normally attach (get a renderer) during style-resolve, which is a precursor to layout.

An element normally has a renderer, but elements can explicitly opt out (by being display: none) or a few other reasons:

bool NodeRenderingContext::shouldCreateRenderer() const
{
    if (!m_node->document()->shouldCreateRenderers())
        return false;
    if (!m_parentDetails.node())
        return false;
    RenderObject* parentRenderer = this->parentRenderer();
    if (!parentRenderer)
        return false;
    if (!parentRenderer->canHaveChildren())
        return false;
    if (!m_parentDetails.node()->childShouldCreateRenderer(*this))
        return false;
    return true;
}

[Eric Seidel (no email)]

The parser uses a mechanism called "lazy attach" where things get marked as being attached, but aren't yet. :)  It's possible that lazyAttach could be getting confused in this specific case.... but I doubt it.

[Eric Seidel (no email)]

I wonder if we've necessarily finished parsing the document when the worker finishes?  I suspect that could effect the output if not.

[Eric Seidel (no email)]

Yeah, I suspect that's it.  It's not immediately clear to me if this is expected behavior from TextIterator or not, but I suspect if you make the worker-handling happen after the onload instead of inline in a blocking external script, then the behavior should be consistent.

ok, so here's what happens:

when a new WebView is created in chromium, it has an initial windowRect of (0, 0). When this is not updated in time to the correct value, the test tries to squeeze in the text into an empty window which breaks after every word, resulting in the difference.

[Eric Seidel (no email)]

And wrapping after every letter causes the trailing whitespace to appear on innerText?

I thought you said this occurred even w/o your patch?

Does this repro using a 1x1 iframe?

(In reply to comment #13)
> And wrapping after every letter causes the trailing whitespace to appear on innerText?

it wraps after every words, and that causes the trailing whitespace to not appear, yes

> 
> I thought you said this occurred even w/o your patch?

There's always a short time window between the creation of a web contents and when the window rect is set.

> 
> Does this repro using a 1x1 iframe?

I'll try.

[Ryosuke Niwa]

Is this bug still valid?

(In reply to comment #15)
> Is this bug still valid?
no, closing