1531693 - With QuantumBar enabled, private browsing tests throw "Assertion failure: editorBase == aEditorBase (Another editor handled the composition?) TextComposition.cpp:655"

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Mark Banner (:standard8)]

QuantumBar is a rewrite of the address bar that avoids using the existing urlbar xml bindings, and makes the architecture more flexible for future changes.

When running the private browsing tests on try, we're seeing:

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231206885&repo=try&lineNumber=10301
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231207163&repo=try&lineNumber=12889
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231208182&repo=try&lineNumber=29613
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231208182&repo=try&lineNumber=29613
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231208226&repo=try&lineNumber=19853

[INFO - GECKO(3607) | Assertion failure: editorBase == aEditorBase (Another editor handled the composition?), at /builds/worker/workspace/build/src/dom/events/TextComposition.cpp:655
[INFO - GECKO(3607) | #01: mozilla::EditorBase::RemoveEventListeners() [xpcom/base/nsCOMPtr.h:412]
[INFO - GECKO(3607) | #02: mozilla::TextEditor::~TextEditor() [mfbt/RefPtr.h:291]
[INFO - GECKO(3607) | #03: mozilla::TextEditor::~TextEditor() [memory/mozalloc/mozalloc.h:151]
[INFO - GECKO(3607) | #04: SnowWhiteKiller::MaybeKillObject(SnowWhiteKiller::SnowWhiteObject&) [xpcom/base/nsCycleCollector.cpp:0]
[INFO - GECKO(3607) | #05: SnowWhiteKiller::~SnowWhiteKiller() [xpcom/base/nsCycleCollector.cpp:2414]
[INFO - GECKO(3607) | #06: nsCycleCollector::FreeSnowWhite(bool) [mfbt/SegmentedVector.h:139]
[INFO - GECKO(3607) | #07: nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) [mfbt/RefPtr.h:291]
[INFO - GECKO(3607) | #08: nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [xpcom/base/nsCycleCollector.cpp:3407]
[INFO - GECKO(3607) | #09: nsCycleCollector_collectSlice(js::SliceBudget&, bool) [xpcom/base/nsCycleCollector.cpp:3955]
[INFO - GECKO(3607) | #10: nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) [dom/base/nsJSEnvironment.cpp:1475]
[INFO - GECKO(3607) | #11: CCRunnerFired(mozilla::TimeStamp) [dom/base/nsJSEnvironment.cpp:0]
[INFO - GECKO(3607) | #12: mozilla::IdleTaskRunner::Run() [xpcom/threads/IdleTaskRunner.cpp:63]
[INFO - GECKO(3607) | #13: mozilla::TimedOut(nsITimer*, void*) [mfbt/RefPtr.h:75]

On linux 32 we see a leak:

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231207002&repo=try&lineNumber=10103

For the assertion, it seems to happen outside of any tests being run and before shutdown, so I assume that this correlates to the cycle collector being in the stack.

For QuantumBar, the base urlbar binding for the text box is very similar, but it is wrapped by UrlbarInput

Any help with understanding what's happening here would be appreciated.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Maybe Editor, or maybe the QuantumBar itself, if it is using some editing APIs.

Comment 2

[Drew Willcoxon :adw]

Quantumbar does use editor APIs when it formats its input's value. It uses editor.selectionController and editor.rootElement, in this module: https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/browser/components/urlbar/UrlbarValueFormatter.jsm

It also listens for composition events, in case that's related. The top of the stack in comment 0 is mozilla::EditorBase::RemoveEventListeners(), so maybe it is?

But the existing awesomebar uses that same module and also listens for composition events and doesn't have these errors (I presume?).

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Masayuki knows Editor well and may have ideas for comment 0 :)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

The assertion means that there is a composition in the destroying or reframing editor. However, the TextComposition::mEditorBaseWeak has already been set to nullptr or another editor instance. I think that the latter cause is impossible, but the previous may occur with cycle collection.

What happens if you change here from:

  MOZ_ASSERT(editorBase == aEditorBase,
             "Another editor handled the composition?");

to:

  MOZ_ASSERT(!editorBase || editorBase == aEditorBase,
             "Another editor handled the composition?");

Even if it fixes hitting the assertion, it shouldn't happen though...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Even if it fixes hitting the assertion, it shouldn't happen though...

Ah, it occurs while TextEditor instance is being destroyed by the cycle collection. So, if weak reference from TextComposition is removed first (I guess should be so), comment 4 may be the right fix.

Comment 6

[Mark Banner (:standard8)]

Taking whilst I look into Masayuki's suggestions.

Comment 7

[Mark Banner (:standard8)]

So I did some digging and changing the assertion, per Masayuki's suggestions, stops the tests aborting, but they were still leaking:

 ERROR TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js | leaked 1 window(s) until shutdown [url = about:blank]
 ERROR TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xul]
 INFO TEST-INFO | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js | windows(s) leaked: [pid = 9604] [serial = 14], [pid = 9604] [serial = 13]
 ERROR TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js | leaked 1 docShell(s) until shutdown

After some debugging into that, I eventually realised this was because we are kicking off a search, then immediately closing the private browsing window. Whilst I don't understand exactly why that's causing a leak, I think that's unlikely to happen in real life.

In one of the tests, we're actually assuming that the search starts and doesn't get around to opening the pop-up before pressing escape - which could be a little bit intermittent inducing (though none filed atm).

Anyway, with this fixed, we're looking good on the test front for the private browsing tests (https://treeherder.mozilla.org/#/jobs?repo=try&revision=776bafe4f71ee569f672184a0839a20189647892 - the oranges there are other QuantumBar tests failing that we're still fixing).

Comment 8

[Mark Banner (:standard8)]

Attachment: Bug 1531693 - Relax the assertion for another editor handling the composition to take account of cycle collection. r?masayuki

Comment 9

[Mark Banner (:standard8)]

Attachment: Bug 1531693 - For the PrivateBrowsing about page test, ensure we wait for autocomplete to finish. r?mak

This ensures test stability, and avoids leaking browser windows when we're closing the private browsing window in the test.

Depends on D24529

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf5460df1909
Relax the assertion for another editor handling the composition to take account of cycle collection. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/4d2429507163
For the PrivateBrowsing about page test, ensure we wait for autocomplete to finish. r=mak

Comment 11

[Oana Pop-Rus]

Backed out 2 changesets (bug 1531693) for bc failures browser_privatebrowsing_about.js

Backout: https://hg.mozilla.org/integration/autoland/rev/20f26343fafef69fc9f6f3f37cf984432da290bd

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=4d24295071637190e08340a15834b03f7416fc09&selectedJob=235804731

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=235804731&repo=autoland&lineNumber=1572

Comment 12

[Mark Banner (:standard8)]

I ended up disabling waiting for the search autocomplete for AwesomeBar because of bug 1539199, that shouldn't matter much as this just reverts that part of the test to the same as it was (for AwesomeBar), for QuantumBar this should still work as well.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8dcb951f831
Relax the assertion for another editor handling the composition to take account of cycle collection. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/0969fd6383ce
For the PrivateBrowsing about page test, ensure we wait for autocomplete to finish. r=mak

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/d8dcb951f831
https://hg.mozilla.org/mozilla-central/rev/0969fd6383ce

Comment 15

[Julien Cristau [:jcristau]]

I'm assuming this is fine to ride the trains.