Closed Bug 1403026 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mrbkap, Assigned: masayuki)

References

Details

Attachments

(1 file)

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
Flags: needinfo?(masayuki)
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?
Flags: needinfo?(masayuki) → needinfo?(mrbkap)
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.
Flags: needinfo?(mrbkap)
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
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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.
Attachment #8913237 - Flags: review?(bugs) → review+
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.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/322eba7fa97a
Make IMEStateManager release all string buffer of its static members at XPCOM shutdown r=smaug
https://hg.mozilla.org/mozilla-central/rev/322eba7fa97a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
> > +    // 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?
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
(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.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: