1403026 - XPCOM objects created/destroyed from static ctor/dtor caused by InputContext

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

There's a new instance of the warning in the summary in the parent process. The stack is:

  * frame #0: 0x0000000101dbba7d XUL`AssertActivityIsLegal() at nsTraceRefcnt.cpp:171
    frame #1: 0x0000000101daefbc XUL`::NS_LogRelease(aPtr=0x000000014138ab80, aRefcnt=0, aClass="nsStringBuffer") at nsTraceRefcnt.cpp:1066
    frame #2: 0x0000000101d2f584 XUL`nsTString<char16_t>::~nsTString(this=0x000000010f1fb320) at nsStringFwd.h:29
    ...
    frame #9: 0x000000010407fa75 XUL`mozilla::widget::InputContext::~InputContext(this=0x000000010f1fb318) at IMEData.h:273
    frame #10: 0x00007fffb8cac178 libsystem_c.dylib`__cxa_finalize_ranges + 332
    frame #11: 0x00007fffb8cac4b2 libsystem_c.dylib`exit + 55

Looking at the patch from bug 1396725, I see [1], which would certainly cause this. We should do something like bug 1401412 or find another way of tearing down this object before XPCOM is torn down.

[1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/events/IMEStateManager.h#356

Comment 1

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

Well, nsString instances of InputContext may be not empty even after XPCOM shutdown. Is it enough to make the nsString objects empty at XPCOM shutdown? Or do we need to destroy the string instances completely?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I *think* that it might be enough to clear the strings before shutdown, but I'm not sure. I think it would be cleaner to destroy the string objects entirely.

Comment 3

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

Thanks. The nsString instances are member of InputContext and InputContext instance is not allocated dynamically. So, the simplest solution is, calling SetCapacity(0).
https://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/xpcom/string/nsTSubstring.cpp#713,718,720

Then, their flags are set to TERMINATED:
https://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/xpcom/string/nsTSubstring.h#845,849

And that'll avoid to run dtor after XPCOM shutdown:
https://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/xpcom/string/nsSubstring.cpp#110,112,114,121

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=311163d5a2749bd33b2e749880c33db9d497a62e

Comment 5

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

Attachment: Bug 1403026 - Make IMEStateManager release all string buffer of its static members at XPCOM shutdown

IMEStateManager has a static instance of InputContext which has some nsString
instances.  Then, their members will be released after XPCOM shutdown and
that is detected as unexpected destruction.

Therefore, IMEStateManager should release them by itself at XPCOM shutdown.
Using nsTSubstring::SetCapacity(0) is the simplest way to release only the
string buffers.

Review commit: https://reviewboard.mozilla.org/r/184636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184636/

Comment 6

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

Comment on attachment 8913237 [details]
Bug 1403026 - Make IMEStateManager release all string buffer of its static members at XPCOM shutdown

https://reviewboard.mozilla.org/r/184636/#review189840

::: widget/IMEData.h:289
(Diff revision 1)
> +  // detected as memory leak.
> +  void ShutDown()
> +  {
> +    // The buffer for nsString will be released with a call of SetCapacity(0).
> +    // Truncate() isn't enough because it just sets length to 0.
> +    mHTMLInputType.SetCapacity(0);

I guess another option is to explicitly assign EmptyString() to these variables. Up to you.

Comment 7

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

Comment on attachment 8913237 [details]
Bug 1403026 - Make IMEStateManager release all string buffer of its static members at XPCOM shutdown

https://reviewboard.mozilla.org/r/184636/#review189840

> I guess another option is to explicitly assign EmptyString() to these variables. Up to you.

Yeah, but I'm afraid the performance of constructing new (temporary?) instance.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/322eba7fa97a
Make IMEStateManager release all string buffer of its static members at XPCOM shutdown r=smaug

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/322eba7fa97a

Comment 10

[Henri Sivonen (:hsivonen)]

> > +    // The buffer for nsString will be released with a call of SetCapacity(0).
> > +    // Truncate() isn't enough because it just sets length to 0.

By code inspection, Truncate() / SetLength(0) appears to have released the buffer since at least February 2004 / bug 231995. Was the above comment based on testing indicating behavior to the contrary?

Comment 11

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

(In reply to Henri Sivonen (:hsivonen) from comment #10)
> > > +    // The buffer for nsString will be released with a call of SetCapacity(0).
> > > +    // Truncate() isn't enough because it just sets length to 0.
> 
> By code inspection, Truncate() / SetLength(0) appears to have released the
> buffer since at least February 2004 / bug 231995. Was the above comment
> based on testing indicating behavior to the contrary?

IIRC, I understood it as so when I investigating the string classes. But I check it now, indeed, Trancate() calls SetCapacity(). So, it may be just wrong comment.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #11)
> (In reply to Henri Sivonen (:hsivonen) from comment #10)
> > > > +    // The buffer for nsString will be released with a call of SetCapacity(0).
> > > > +    // Truncate() isn't enough because it just sets length to 0.
> > 
> > By code inspection, Truncate() / SetLength(0) appears to have released the
> > buffer since at least February 2004 / bug 231995. Was the above comment
> > based on testing indicating behavior to the contrary?
> 
> IIRC, I understood it as so when I investigating the string classes. But I
> check it now, indeed, Trancate() calls SetCapacity(). So, it may be just
> wrong comment.

Thanks. I can proceed as planned with bug 1487341 then.