952570 - Weak tickler pointer is unsafe

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Josh Matthews [:jdm]]

The mTickler weak pointer member in TicklerTimer isn't safe, because do_QueryReferent cannot be used to convert a weak pointer into a concrete C++ type. It happens to work because Ticker only inherits from nsISupports, however this means that if any other nsISupports-derived pointer was stored in mTickler instead, the query would "succeed", but any operations on the resulting concrete Tickler type would be unsafe. The correct fix here is to switch from nsWeakPtr/nsSupportsWeakReference to mfbt's WeakPtr and SupportsWeakPtr types instead (http://mxr.mozilla.org/mozilla-central/source/mfbt/WeakPtr.h).

Comment 1

[Josh Matthews [:jdm]]

The problematic code lives in http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.cpp and http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.h .

Comment 2

[Craig MacKenzie [:gentlefolk]]

I'd like to take this on as a first bug if possible.

Comment 3

[Josh Matthews [:jdm]]

It's yours! Please ask questions if anything in comment 0 or comment 1 is unclear.

Comment 4

[Craig MacKenzie [:gentlefolk]]

Thanks! Are there tests I can use to verify my changes or is this going to be a "correct by design" sort of fix?

Comment 5

[Josh Matthews [:jdm]]

Looks like the latter. This seems to only be used on Android, and seems to be tied into the general operation of the networking layer.

Comment 6

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_ticklerpointer.diff

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 8351401 [details] [diff] [review]
bug952570_ticklerpointer.diff

Review of attachment 8351401 [details] [diff] [review]:
-----------------------------------------------------------------

We'll have a peer of the network module review this once you address my comment. Nice job!

::: netwerk/base/src/Tickler.cpp
@@ -258,5 @@
>  NS_IMPL_ISUPPORTS1(TicklerTimer, nsITimerCallback)
>  
>  NS_IMETHODIMP TicklerTimer::Notify(nsITimer *timer)
>  {
> -  nsRefPtr<Tickler> tickler = do_QueryReferent(mTickler);

I would be inclined to leave the nsRefPtr and maintain a strong reference to the original tickler object to ensure that it remains alive throughout this method.

Comment 8

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_ticklerpointer.diff - Addressed feedback from comment 7

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 8351405 [details] [diff] [review]
bug952570_ticklerpointer.diff - Addressed feedback from comment 7

Review of attachment 8351405 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Tickler.h
@@ +53,5 @@
>  namespace net {
>  
>  #ifdef MOZ_USE_WIFI_TICKLER
>  
> +class Tickler MOZ_FINAL : public nsSupportsWeakReference,

Oh, I didn't notice this before. We shouldn't inherit from this type any longer, and I don't think we need to include nsWeakReference.h either.

Comment 10

[Josh Matthews [:jdm]]

If there are build errors as a result, look into the NS_INLINE_DECL_REFCOUNTING macro: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#362

Comment 11

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_ticklerpointer.diff - Addressed feedback from comment 9

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 8351408 [details] [diff] [review]
bug952570_ticklerpointer.diff - Addressed feedback from comment 9

Review of attachment 8351408 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Tickler.h
@@ -57,3 @@
>  {
>  public:
>    NS_DECL_THREADSAFE_ISUPPORTS

Ack, I apologize for the multiple passes here; this line should change to the macro I suggested before, and http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.cpp#21 will need to go away.

Comment 13

[Craig MacKenzie [:gentlefolk]]

Should I replace the line above with http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#362 or http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#397?

There are two additional occurrences of NS_DECL_THREADSAFE_ISUPPORTS of at:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.cpp#2179
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.h#106

Should we replace the above as well?

Comment 14

[Craig MacKenzie [:gentlefolk]]

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.cpp#2179 should be http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.cpp#217

Comment 15

[Josh Matthews [:jdm]]

Good catches. If you're replacing a THREADSAFE_ISUPPORTS, use the equivalent THREADSAFE_REFCOUNTING. You should go ahead and replace it in both Tickler definitions, and make the other one not inherit from anything. With regards to the TicklerTimer, that should stay as it is, since it's an XPCOM timer implementation and there's nothing incorrect about it.

Comment 16

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_ticklerpointer.diff - Addressed feedback from comment 12

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 8351410 [details] [diff] [review]
bug952570_ticklerpointer.diff - Addressed feedback from comment 12

Review of attachment 8351410 [details] [diff] [review]:
-----------------------------------------------------------------

You can make the next review go to :mcmanus, who originally created this file according to hg log. Thanks!

::: netwerk/base/src/Tickler.h
@@ +57,3 @@
>  {
>  public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Ticker)

s/Ticker/Tickler/

Comment 18

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_ticklerpointer.diff - Fixed all nitial review comments

Comment 19

[Patrick McManus [:mcmanus]]

Craig, jdm - happy new year and thanks for the patch!

Comment 20

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8df72b4ee2f1

Comment 21

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9f1067aea9

Whoops, I should have run it through tryserver first. The android build is broken by these changes; Craig, could you look at the logs at https://tbpl.mozilla.org/php/getParsedLog.php?id=32477609&tree=Mozilla-Inbound and fix the patch up?

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 8351420 [details] [diff] [review]
bug952570_ticklerpointer.diff - Fixed all nitial review comments

Review of attachment 8351420 [details] [diff] [review]:
-----------------------------------------------------------------

Are we sure using the weakptr is thread-safe?  this is apparently being ran on multiple threads.  and AFAIK the code for WeakPtr is definitely not thread safe.

::: netwerk/base/src/Tickler.cpp
@@ +290,2 @@
>  } // namespace mozilla::net
>  } // namespace mozilla

so this whole block can be now removed?

Comment 23

[Josh Matthews [:jdm]]

(In reply to Honza Bambas (:mayhemer) from comment #22)
> Comment on attachment 8351420 [details] [diff] [review]
> bug952570_ticklerpointer.diff - Fixed all nitial review comments
> 
> Review of attachment 8351420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are we sure using the weakptr is thread-safe?  this is apparently being ran
> on multiple threads.  and AFAIK the code for WeakPtr is definitely not
> thread safe.

What specific concerns do you have? I don't see any code dealing with threading concerns in nsWeakReference/nsSupportsWeakReference either.

Comment 24

[Honza Bambas (:mayhemer)]

recently I found out that we are using nsWeakPtr in the new cache wrong and that it is probably a cause of crashes.

Scenario:
- thread A drops the refcnt to 0, enters the branch that does |delete this|
- before thread A reaches the |delete this| line thread B takes and addrefs the referent from weakreference object since we set the *referent* t o NULL from the object's dtor that has not been reached yet
- thread A calls |delete this|
- thread B works on a deleted object and when it's released it may be (depending on in what state the heap is) double deleted


Makes sense?

Comment 25

[Craig MacKenzie [:gentlefolk]]

Re: Comment 21 - I'll look into the android build failures.

Comment 26

[Craig MacKenzie [:gentlefolk]]

Re: Comment 21:

The issue with the android build was the change made to address Comment 7. See https://bugzilla.mozilla.org/attachment.cgi?id=8351420&action=diff#a/netwerk/base/src/Tickler.cpp_sec4.

The ARM compiler is complaining that we cannot assign a WeakPtr directly to an nsRefPtr object. This can be corrected with a cast, but since I'm not very familiar with the reference counting objects in use I'd like some input on whether this is a reasonable thing to do.

Comment 27

[Josh Matthews [:jdm]]

I think |nsRefPtr<Tickler> tickler(mTickler)| should coerce properly.

Comment 28

[Josh Matthews [:jdm]]

Comment on attachment 8351420 [details] [diff] [review]
bug952570_ticklerpointer.diff - Fixed all nitial review comments

Sorry, Craig, but Honza's comment and bug 956327 necessitate a different solution for this bug. The code that exists is still unsafe, but our weak pointer implementations don't handle multiple threads correctly. The solution we should implement is to add an IID to Tickler (NS_DECLARE_STATIC_IID_ACCESSOR and NS_DEFINE_STATIC_IID_ACCESSOR), and change the NS_IMPL_ISUPPORTS1 to add Tickler to the list. This should make the nsRefPtr usage correct, and we'll address the threading problems elsewhere.

Comment 29

[Craig MacKenzie [:gentlefolk]]

Sure, I'll put together a new patch implementing your suggestions above.

Comment 30

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_tickleriid.diff - Addressed feedback from comment 28

Comment 31

[Josh Matthews [:jdm]]

Comment on attachment 8355932 [details] [diff] [review]
bug952570_tickleriid.diff - Addressed feedback from comment 28

This is missing a change to NS_IMPL_ISUPPORTS, otherwise nothing is actually using the new IID.

Comment 32

[Craig MacKenzie [:gentlefolk]]

Ah, I misinterpreted that as reinstating the line that was removed in the original patch. How should I modify the existing ISUPPORTS? It isn't clear to me what specifically needs to change.

Comment 33

[Josh Matthews [:jdm]]

If there's an NS_IMPL_ISUPPORTSN, it should become NS_IMPL_ISUPPORTS(N+1) with an additional argument of Tickler. This indicates that attempts to query for the new Tickler IID should succeed and return the Tickler class.

Comment 34

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_tickleriid.diff - Addressed feedback from comment 33

Comment 35

[Josh Matthews [:jdm]]

Comment on attachment 8356284 [details] [diff] [review]
bug952570_tickleriid.diff - Addressed feedback from comment 33

> NS_IMPL_ISUPPORTS1(Tickler, nsISupportsWeakReference)
>+NS_IMPL_ISUPPORTS2(Tickler)

Sorry, this won't build :) 
NS_IMPL_ISUPPORTS2(Tickler, nsISupportsWeakReference, Tickler) should replace the existing NS_IMPL_ISUPPORTS1

Comment 36

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_tickleriid.diff - Addressed feedback from comment 35

Comment 37

[Josh Matthews [:jdm]]

Comment on attachment 8356321 [details] [diff] [review]
bug952570_tickleriid.diff - Addressed feedback from comment 35

I'll throw this at the tryserver and make sure it builds.

Comment 38

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=cf5d3319ed61

Comment 39

[Josh Matthews [:jdm]]

Whoops, build error on Android, since we're missing an actual definition of NS_TICKLER_IID. See http://mxr.mozilla.org/mozilla-central/source/accessible/public/nsIAccessibilityService.h#29 for an example of one; just run `mach uuid` and split the result up into the appropriate groupings for Tickler's.

(oh, and be sure to mark old patches as obsolete when you upload new ones)

Comment 40

[Craig MacKenzie [:gentlefolk]]

Attachment: bug952570_tickleriid.diff - Fixed build error on Android (comment 39)

Comment 41

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=e27e61283f20

Comment 42

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc819f136ef8

Comment 43

[Josh Matthews [:jdm]]

Comment on attachment 8358105 [details] [diff] [review]
bug952570_tickleriid.diff - Fixed build error on Android (comment 39)

Review of attachment 8358105 [details] [diff] [review]:
-----------------------------------------------------------------

Whoops, forgot the r+.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc819f136ef8