1171712 - crash in InkCollector::Release()

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]: topcrash in v41

This bug was filed from the Socorro interface and is 
report bp-6e19094b-fb22-493e-bfae-eef6c2150603.
=============================================================

Recent topcrash regression in v41, starting in build 20150602055237.

Perhaps bug 1164473?

Comment 2

[Jim Mathies [:jimm]]

Top crash? I see only 49 reports.

https://crash-stats.mozilla.com/report/list?product=Firefox&signature=InkCollector%3A%3ARelease%28%29

Comment 3

[Jim Mathies [:jimm]]

Regardless, maksim, we need to track this down and fix it.

Comment 4

[Mats Palmgren (inactive)]

(In reply to Jim Mathies [:jimm] from comment #2)
> Top crash? I see only 49 reports.

This crash is currently at #21 in v41 crash data.  Don't be mislead by the relatively
low volume - that's due to having few Nightly users.  Crashes on this list are likely
to become top crashers also when they reach other channels (and then the absolute
volume will go up of course).

Comment 5

[Maksim Lebedev]

Some investigation:
Very strange that such crash we can reproduce only at Win7 platform. At Win8 there are no craches with such callstack. The main difference is located at behavior when PEN leaves hover of digitizer.

On Win8:
InkCollector::QueryInterface(DIID__IInkCollectorEvents)
-> NS_ADDREF_THIS()
InkCollector::Invoke(DISPID_ICECursorOutOfRange)
-> NS_RELEASE_THIS()
That's why behavior is correct, and we cannot see any crashes.

On Win7:
InkCollector::Invoke(DISPID_ICECursorOutOfRange)
-> NS_RELEASE_THIS()
That's why after some times of repeating we get InkCollector::~InkCollector()

Comment 6

[Maksim Lebedev]

Some investigation: On Win7:
InkCollector::QueryInterface(DIID__IInkCollectorEvents)
-> NS_ADDREF_THIS()   [++mRefCount]
InkCollector::Invoke(DISPID_ICECursorOutOfRange)
-> NS_RELEASE_THIS()   [--mRefCount]
InkCollector::Release()   [--mRefCount]

Every time, release calls twice, but addref only one.

Additional investigation: Sequence:
InkCollector::~InkCollector()
 -> mConnectionPoint->Unadvise()
  -> InkCollector::Release()
   -> InkCollector::~InkCollector() again.
As result we have exception. Looks like it happens only at Win7. I am surprized...

Comment 7

[Maksim Lebedev]

Attachment: separated_ink_collector_ver1.diff

+ Added separated InkCollector class
+ Added InkCollector::GetTarget()
+ All functions are divided between InkCollector and InkCollectorEvent classes
+ StaticRefPtr<*> InkCollector::sInkCollector -> UniquePtr<*> InkCollector::sInkCollector
+ Added using SUCCEEDED without explicity comparisons with NS_OK
- Removed info about about IID_IMarshal interface at InkCollectorEvent::QueryInterface ()
- Removed releasing object at InkCollectorEvent::Invoke()
+ Moved calling ClearTarget from InkCollectorEvent::Invoke() -> nsWindow::ProcessMessage()

Suggestions and comments and objections are very welcome.

Comment 8

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

Comment on attachment 8620468 [details] [diff] [review]
separated_ink_collector_ver1.diff

>+UniquePtr<InkCollector> InkCollector::sInkCollector;
Oh, InkCollector isn't refcounted anymore. What ensures sInkCollector is cleared early enough during shutdown process that
we don't get shutdown leaks? Did you test this patch on debug build?

> 
>+  // Set up instance of InkCollectorEvent.
>+  mInkCollectorEvent = new InkCollectorEvent();
>+  NS_ADDREF(mInkCollectorEvent);
Wait, why this extra addref? mInkCollectorEvent addrefs already

>-  if (FAILED(::CoCreateFreeThreadedMarshaler(this, getter_AddRefs(mMarshaller)))) {
>+  if (FAILED(::CoCreateFreeThreadedMarshaler(mInkCollectorEvent, getter_AddRefs(mMarshaller)))) {
>     return;
>   }
>+  NS_ADDREF(mMarshaller);
same here

>+
>   // Create the ink collector.
>   if (FAILED(::CoCreateInstance(CLSID_InkCollector, NULL, CLSCTX_INPROC_SERVER,
>                                 IID_IInkCollector, getter_AddRefs(mInkCollector)))) {
>     return;
>   }
>   NS_ADDREF(mInkCollector);
hmm, why do we have this? Really odd.

>+
>   // Set up connection between sink and InkCollector.
>   nsRefPtr<IConnectionPointContainer> connPointContainer;
>   // Get the connection point container.
>   if (SUCCEEDED(mInkCollector->QueryInterface(IID_IConnectionPointContainer,
>                                               getter_AddRefs(connPointContainer)))) {
>     // Find the connection point for Ink Collector events.
>     if (SUCCEEDED(connPointContainer->FindConnectionPoint(__uuidof(_IInkCollectorEvents),
>                                                           getter_AddRefs(mConnectionPoint)))) {
>       NS_ADDREF(mConnectionPoint);
and this.
Or does Windows API not addref those out params? If so, getter_AddRefs shouldn't be used, because it
tells to the reader that addref has happened

>   switch (aDispIdMember) {
>     case DISPID_ICECursorOutOfRange: {
>       if (aDispParams && aDispParams->cArgs) {
>         CursorOutOfRange(static_cast<IInkCursor*>(aDispParams->rgvarg[0].pdispVal));
>-        ClearTarget();
>+        //ClearTarget();
what is this?

>       }
>       break;
>     }
>   };
>   // Release should be called after all usage of this method.
>-  NS_RELEASE_THIS();
>+  //NS_RELEASE_THIS();
what is this?

Comment 9

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #8)
> Oh, InkCollector isn't refcounted anymore. What ensures sInkCollector is 
> cleared early enough during shutdown process that we don't get shutdown leaks?
InkCollector is created only once, and is destroyed only once. It is not needed to be refCounted.
> Did you test this patch on debug build?
Yes, of course, certainly.
> >+  NS_ADDREF(mInkCollectorEvent);
> Wait, why this extra addref? mInkCollectorEvent addrefs already
For using corresponding NS_IF_RELEASE(mInkCollectorEvent) in destructor,
but looks like it can be removed.
> >   switch (aDispIdMember) {
> >     case DISPID_ICECursorOutOfRange: {
> >       if (aDispParams && aDispParams->cArgs) {
> >         CursorOutOfRange(static_cast<IInkCursor*>(aDispParams->rgvarg[0].pdispVal));
> >-        ClearTarget();
> >+        //ClearTarget();
> what is this?
Looks like ClearTarget() should be called only at main thread to prevent crashes.
> >   // Release should be called after all usage of this method.
> >-  NS_RELEASE_THIS();
> >+  //NS_RELEASE_THIS();
> what is this?
It is not needed now. Testing showed that refCount is not increased now.

Comment 10

[Maksim Lebedev]

Attachment: separated_ink_collector_ver2.diff

- Removed unwanted source code.

Suggestions and comments and objections are very welcome.

Comment 11

[Maksim Lebedev]

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

Comment 12

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

(In reply to Maksim Lebedev from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > Oh, InkCollector isn't refcounted anymore. What ensures sInkCollector is 
> > cleared early enough during shutdown process that we don't get shutdown leaks?
> InkCollector is created only once, and is destroyed only once. It is not
> needed to be refCounted.
But what guarantees it is deleted early enough during shutdown process that we don't end up showing leaks in
debug builds?

Comment 13

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

Comment on attachment 8620966 [details] [diff] [review]
separated_ink_collector_ver2.diff

Of we do set sInkCollector to null in nsWindow::~nsWindow().

But use StaticAutoPtr, not UniquePtr when dealing static global variables.

Comment 14

[Maksim Lebedev]

Attachment: separated_ink_collector_ver3.diff

+ Changed UniquePtr<InkCollector>  ->  StaticAutoPtr<InkCollector>

Suggestions and comments and objections are very welcome.

Comment 15

[Maksim Lebedev]

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

Comment 16

[Maksim Lebedev]

Some additional information: https://bugzilla.mozilla.org/show_bug.cgi?id=1171151#c15

If there are no objections, I put checkin-needed flag.

Comment 17

[Dmitriy Barkalov]

(In reply to Maksim Lebedev from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a08c05fa12ea

Tested Nightly x64 and x32 with multi-process and pan-zoom on and off and pointer events enabled on Windows 7 and Windows 8. 

Crash is not reproduced.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/783aeb240386

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/783aeb240386

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] also shows zero instances of this crash over the past 4 weeks.

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=InkCollector%3A%3ARelease%28%29