Closed Bug 1531693 Opened 6 months ago Closed 5 months ago

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

Categories

(Core :: Editor, defect, P3)

defect
Points:
5

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

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

Component: DOM: Events → Editor

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?).

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

Flags: needinfo?(masayuki)

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...

Flags: needinfo?(masayuki)
Priority: -- → P3

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.

Taking whilst I look into Masayuki's suggestions.

Assignee: nobody → 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).

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

Depends on D24529

Pushed by mbanner@mozilla.com:
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

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.

Flags: needinfo?(standard8)
Pushed by mbanner@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Points: --- → 5

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

You need to log in before you can comment on or make changes to this bug.