Bug 165073 – REGRESSION(r203047): [GTK][Stable] "notify::title" signal unreliably triggered multiple times

• Component: WebKitGTK
• Status: RESOLVED FIXED

Created attachment 295432 [details]
Example Python and HTML to reproduce bug

WebKit2GTK 2.14 (and 2.14.1) has introduced a new bug which causes the "notify::title" signal to be called multiple times then it is necessary, whereas previous versions only trigger once. This is causing a problem for desktop web applications that change the document's title as a bridge between JavaScript and program's code, such as Python.

This issue is not present under v2.12.

Attached to this bug report is a Python example that reproduces this bug, which needs extracting to /tmp as it reads a basic HTML document.

* Under 2.12, clicking the "One", "Two", "Three" links prints "One clicked", "Two clicked", etc in the terminal once, which is the expected behaviour.

* Under 2.14, clicking the links prints "One clicked", but then a subsequent click on "Two" and "Three" results in "xxx clicked", a null, then "xxx clicked" again, which indicates a bug with the notify::title signal.

Thank you in advance.

Originally reported on Launchpad:
https://bugs.launchpad.net/ubuntu/+source/webkit2gtk/+bug/1633736

[Michael Catanzaro]

Hm, surely the title is expected to be unset when a page load is started...? There cannot be any title until after the load is committed, right? I'm surprised by the 2.12 behavior, not the 2.14 behavior (you can simply ignore the null titles in the signal handler if that's what you want to do, right?) Carlos, what's really the expected behavior here?

Epiphany prior to 3.23 relied on seeing the null title to know to switch from title box mode to location entry mode, so we should be careful to check for regressions there if changing this.

Returning null makes sense when the title is changed but remains the same as the last one (e.g. "One" → "One"). Currently, an application would still receive the signal and take action (in this case, print to the terminal) whereas the behaviour in 2.12 did not trigger (a workaround was getting the JavaScript to set the title to null after xx milliseconds).

However, while a null can be ignored, the problem lies that clicking any sequential links does this: "One clicked" → null → "One clicked", causing the application to trigger its action twice.

An ideal solution would be to only notify a title change once, so it's compatible with 2.12 behaviour: "One clicked" → null ... "Two clicked" → null ... etc

[Carlos Garcia Campos]

(In reply to comment #2)
> Returning null makes sense when the title is changed but remains the same as
> the last one (e.g. "One" → "One"). Currently, an application would still
> receive the signal and take action (in this case, print to the terminal)
> whereas the behaviour in 2.12 did not trigger (a workaround was getting the
> JavaScript to set the title to null after xx milliseconds).
> 
> However, while a null can be ignored, the problem lies that clicking any
> sequential links does this: "One clicked" → null → "One clicked", causing
> the application to trigger its action twice.
> 
> An ideal solution would be to only notify a title change once, so it's
> compatible with 2.12 behaviour: "One clicked" → null ... "Two clicked" →
> null ... etc

So, if I understand the thing correctly, with your test case in 2.12 you clicked on one clicked and then you got "One clicked" only, then you clicked on two clicked and you got, null and then "Two Clicked", right? So, the different with 2.14 is the first "Two Clicked" in this case.

If that's the case, I think it has nothing to do with the WebView title property actually, but with the document title handling. My guess is that for whatever reason in 2.12 the title change didn't really happen because of the new load, but in 2.14 the title change is processed before the new load starts. I guess something changed in WebCore regarding the onclick event. Your test cases does:

<a href="#" onclick="cmd('One clicked.')">One</a><br>
<a href="#" onclick="cmd('Two clicked.')">Two</a><br>
<a href="#" onclick="cmd('Three clicked.')">Three</a>

And cmd just changes document.title. So what happens is that onclick events are handled and title changes, from One clicked to Two clicked for example. Then the new load starts and the title is reset on committed, so you get the null. Then the load happens and title is set again. I see your point, though, that if title is going to be the same, we change it for nothing, but on committed the PageLoadObserver doesn't know the title yet. This is not GTK+ specific issue in any case. Maybe Darin or Chris know what changed in WebCore.

[Michael Catanzaro]

(In reply to comment #3)
> So, if I understand the thing correctly, with your test case in 2.12 you
> clicked on one clicked and then you got "One clicked" only, then you clicked
> on two clicked and you got, null and then "Two Clicked", right? So, the
> different with 2.14 is the first "Two Clicked" in this case.

I think he's saying it was never reset to NULL at all in 2.12?

(In reply to comment #4)
> I think he's saying it was never reset to NULL at all in 2.12?

Correct. To help visualise what happens, I've attached two GIFs of the behaviours  under 2.12 (Ubuntu 16.04) versus 2.14 (Ubuntu 16.10).

> I guess something changed in WebCore regarding the onclick event.

Carlos, as a quick test, should the title be set via a JavaScript timeout (and not originating from an "onclick"), the same thing happens as before:

    <script>
        setTimeout(function() { document.title = "Auto One"; }, 1000)
        setTimeout(function() { document.title = "Auto Two"; }, 2000)
        setTimeout(function() { document.title = "Auto Three"; }, 3000)
    </script>

Created attachment 295727 [details]
GIF visualising behaviour under WebKit 2.12

Created attachment 295728 [details]
GIF visualising behaviour under WebKit 2.14

It would be good to know whether this is a bug or an intentional change and what our options are.

In the meantime, I have written a crude workaround (in the application's code) that restores 2.12 behaviour in >=2.14:

====================================

To be placed in __init__ function:

        self.cmd_buffer = []
        self.cmd_set = False

To be placed in 'notify::title' callback function:

        # Ignore NULL
        if len(title) == 0:
            return
        else:
            self.cmd_buffer.append(title)

        # Once the page is loaded, first command works fine.
        # Subsequential clicks on anything else causes 2 callbacks.
        if self.cmd_set:
            if not len(self.cmd_buffer) >= 2:
                return
        self.cmd_buffer = []
        self.cmd_set = True

To be placed in 'load-changed' callback function:

        self.cmd_set = False

====================================

This comes at with the risk the app may ignore further 'notify::title' signals unless the user clicks a button (in-app) again. This will also be apparent if the behaviour changes again in WebKit2GTK, or a user has an older version of WebKit2. At least now a "command" only runs once.

This workaround has been applied to these 2 apps:
 * https://github.com/lah7/polychromatic/commit/cbde66e9cc84d86485c4f8b8c57b3b5aa0dceb9c
 * https://bitbucket.org/ubuntu-mate/ubuntu-mate-welcome/commits/5210c5dd2567d8a66217819e0784932a60e7c36d?at=master

Ubuntu 16.04 has also updated the packages to 2.14, so any other WebKit2 GTK-based application may want to be wary of the changes to this 'notify::title' signal.

[Darin Adler]

I seem to recall fixing a bug that was resulting in extra title change notifications. It’s possible this is already fixed on TOT.

[Michael Catanzaro]

(In reply to comment #9)
> I seem to recall fixing a bug that was resulting in extra title change
> notifications. It’s possible this is already fixed on TOT.

I've confirmed that this is indeed fixed in trunk.

I glanced over the short summaries of all your commits since 2.14 was branched on August 30 in Trac, trying to find which commit fixed this, but I didn't spot one that looked likely.

[Darin Adler]

The fix was part of the large patch from bug 166569. My apologies for landing this along with a lot of other refactoring.

You can see the relevant change in <https://trac.webkit.org/changeset/210216/trunk/Source/WebCore/dom/Document.cpp>,

    (WebCore::Document::setTitle): Removed code that unnecessarily calls
    updateTitle twice; once indirectly by calling setTextContent on the
    title element and once by directly calling updateTitle.

I thought that was a long-standing bug when I fixed it, I did not realize it was something introduced within the last year. The fix is visible to relevant layout tests: <https://trac.webkit.org/changeset/210216/trunk/LayoutTests/fast/dom/title-text-property-2-expected.txt> had an extra "setting document.title" in it and <https://trac.webkit.org/changeset/210216/trunk/LayoutTests/http/tests/globalhistory/history-delegate-basic-title-expected.txt> shows the progression too.

Looking at the change history for the globalhistory test case, I can see that we introduced the bug when fixing some non-standard behavior in our handling of document.title in <https://trac.webkit.org/changeset/203047>. At that time we missed the fact that the change in test results indicated a bug!

[Michael Catanzaro]

Ah wow, that fix was... really hidden quite well. OK, thanks for the help.

It took some squinting for me to see the fix, but it's straightforward. On our 2.14 branch, at the bottom of Document::setTitle, we have this:

    // The DOM API has no method of specifying direction, so assume LTR.
    updateTitle(StringWithDirection(title, LTR));

    if (is<HTMLTitleElement>(m_titleElement.get()))
        downcast<HTMLTitleElement>(*m_titleElement).setTextContent(title, ASSERT_NO_EXCEPTION);
    else if (is<SVGTitleElement>(m_titleElement.get()))
        downcast<SVGTitleElement>(*m_titleElement).setTextContent(title, ASSERT_NO_EXCEPTION);

We just need to move updateTitle from the top into an else clause. It should look like this:

    if (is<HTMLTitleElement>(m_titleElement.get()))
        downcast<HTMLTitleElement>(*m_titleElement).setTextContent(title, ASSERT_NO_EXCEPTION);
    else if (is<SVGTitleElement>(m_titleElement.get()))
        downcast<SVGTitleElement>(*m_titleElement).setTextContent(title, ASSERT_NO_EXCEPTION);
    else
         updateTitle(StringWithDirection(title, LTR));

[Michael Catanzaro]

(In reply to comment #8)
> This workaround has been applied to these 2 apps:
>  *
> https://github.com/lah7/polychromatic/commit/
> cbde66e9cc84d86485c4f8b8c57b3b5aa0dceb9c
>  *
> https://bitbucket.org/ubuntu-mate/ubuntu-mate-welcome/commits/
> 5210c5dd2567d8a66217819e0784932a60e7c36d?at=master
> 
> Ubuntu 16.04 has also updated the packages to 2.14, so any other WebKit2
> GTK-based application may want to be wary of the changes to this
> 'notify::title' signal.

FYI: we're planning to do a 2.14.3 release hopefully during the next week or two. It will presumably include this fix, since it's pretty simple. You might not want to release that workaround as it looks like that will break when your users get fixed WebKit. (On the other hand, I can't predict when Ubuntu will update to the latest release, but we'll include a good-sized security advisory as encouragement.)

(In reply to comment #13)

Thank you everyone!

> FYI: we're planning to do a 2.14.3 release hopefully during the next week or
> two. It will presumably include this fix, since it's pretty simple.

Good to know, thanks! The Ubuntu maintainer may wish to mark LP: #1633736 as "Fix Released" when it's time: https://bugs.launchpad.net/ubuntu/+source/webkit2gtk/+bug/1633736

[Michael Catanzaro]

Actually I just noticed that the signal is being triggered *three* times per title change in 2.14, and *twice* per title change in trunk. I think this does not makes sense. The problem is here in Document::setTitle:

    downcast<HTMLTitleElement>(*m_titleElement).setTextContent(title);

HTMLTitleElement subclasses HTMLElement, which subclasses StyledElement, which subclasses Element, which subclasses ContainerNode, which subclasses Node. Inside Node::setTextContent, we have this code:

    case DOCUMENT_FRAGMENT_NODE: {
        auto container = makeRef(downcast<ContainerNode>(*this));
        ChildListMutationScope mutation(container);
        container->removeChildren();
        if (text.isEmpty())
            return { };
        return container->appendChild(document().createTextNode(text));
    }

The problem is inside the call to container->removeChildren(). ContainerNode::removeChildren calls HTMLTitleElement::childrenChanged, which calls Document::titleElementTextChanged, which calls Document::updateTitleFromTitleElement, which calls Document::updateTitle. So the title gets unset (set to nullptr). Then HTMLTitleElement::childrenChanged gets called again inside ContainerNode::createTextNode. So each call to Document::setTitle still results in two calls to Document::updateTitle inside HTMLTitleElement::setTextContent.

This is not as serious, since it's (presumably) the behavior we've had for ages. It also makes perfect sense when performing a new page load, since surely we don't want the old page's title to remain after starting a new load.

But I think it doesn't make sense in a case like this:

<script>
    document.title = 'one';
    document.title = 'two';
    document.title = 'three';
</script>

In trunk, this changes the title six times: first to null, then to 'one', then to null again, then to 'two', then to null yet again, then to 'three'. (In 2.14, it changes the title nine times.)

Darin, do you want me to file a new bug report for this, or do you think it's OK as-is?

[Michael Catanzaro]

(In reply to comment #15)
> Darin, do you want me to file a new bug report for this, or do you think
> it's OK as-is?

Bug #167065. I'll include an API test for both issues there.

[Carlos Garcia Campos]

Fixed in r210807.

[Michael Catanzaro]

(In reply to comment #14)
> (In reply to comment #13)
> 
> Thank you everyone!
> 
> > FYI: we're planning to do a 2.14.3 release hopefully during the next week or
> > two. It will presumably include this fix, since it's pretty simple.
> 
> Good to know, thanks! The Ubuntu maintainer may wish to mark LP: #1633736 as
> "Fix Released" when it's time:
> https://bugs.launchpad.net/ubuntu/+source/webkit2gtk/+bug/1633736

And released due to coincidental good timing here: https://webkitgtk.org/2017/01/17/webkitgtk2.14.3-released.html