1184729 - Convert Dispatch/DispatchToMainThread/PutEvent uses to already_AddRefed<>

Status: ASSIGNED

(Core :: XPCOM, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Followup to bug 1155059, now that it's stably on central.  This is to convert the rest of the tree (in a series of patches) to use the new/safer API for Dispatch()/DispatchToMainThread() (and PutEvent() if needed).  DispatchToCurrentThread() doesn't need to be updated for safety, but changing those to already_AddRefed<> will save extra trips through AddRef/Release.

As part of this, we may want to allow (per bug 1183651's discussion) DispatchToMainThread(new Foo(...)) (and Dispatch(new ...))), if we can provide a static assert to block people passing a smart pointer to them.  (This would have to land after the tree is converted over, instead of at that time removing Dispatch(raw_ptr...)).

I plan to land these as a series of patches to different parts of the tree.  Help is welcome!

Common conversions include:
==========================
  nsCOMPtr<nsIRunnable> foo = new bar(..);
  DispatchToMainThread(foo);
->
  DispatchToMainThread(do_AddRef(new bar(...)));
or (with static assertions)
  DispatchToMainThread(new bar(...));


Ditto for nsRefPtr<bar> foo = new bar(...);


  DispatchToMainThread(this);
->
  DispatchToMainThread(do_AddRef(this));


An unusual one:
  nsRefPtr<bar> foo = new bar(...);
  DispatchToMainThread(foo);
  return foo.forget().take();
->
  nsRefPtr<bar> foo = new bar(...);
  DispatchToMainThread(do_AddRef(foo));
  return foo.forget().take();


  nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(...);
  DispatchToMainThread(event);
->
  nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(...);
  DispatchToMainThread(event.forget());
(unless I convert NS_NewRunnableMethod() to return already_AddRefed<>, which would be best but seemed a little tricky.)
or maybe: (with static_asserts)
  DisaptchToMainThread(NS_NewRunnableMethod(...));


Also, converting WrapRunnable() to return already_AddRefed<>, and RUN_ON_THREAD() to take it.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch to block calling NS_DispatchToMainThread with smart pointers

This patch blocks calling NS_DispatchToMainThread with any type of smart pointer.

I tested this patch on MSVC and Clang, and this patch works as expected on both of them. (reports "attempting to reference a deleted function" / "call to deleted function")

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I have done some work on this, so steal this bug. Hopefully you don't mind that.

I see another interesting thing, which is the function NS_ProxyRelease. Many places use this function to ensure some object is only released in the main thread. However, they passed in smart pointers, which are implicitly converted to the raw pointer.

We probably need to do the same thing for that function.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

I'm fine with that; I'll upload my partial patches.  Thanks!

Regarding nsProxyRelease - yes, that should get rewritten.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

BTW, this is currently safe *only* because the event doesn't actually own the reference and instead uses mDoomed->Release() inside of Run(). Since we need to release an arbitrary smartptr (nsRefPtr/nsCOMPtr/RefPtr), it may make sense to keep this part of the impl.

  nsCOMPtr<nsIRunnable> ev = new nsProxyReleaseEvent(aDoomed);
  if (!ev) {
    // we do not release aDoomed here since it may cause a delete on the
    // wrong thread.  better to leak than crash.
    return NS_ERROR_OUT_OF_MEMORY;
  }

  rv = aTarget->Dispatch(ev, NS_DISPATCH_NORMAL);

Here we could be deleting ev either on aTarget or on the current thread

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: DispatchToCurrentThread

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: static assertions to block Dispatch with plain smartptr (WIP, doesn't cover Dispatch() yet)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: SyncRunnable

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WrapRunnable changes

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: media/webrtc changes

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: media/mtransport changes

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/asmjscache

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Apparent real bugs in dom/*

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Possible bugs in dom/*

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/workers

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/various

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/camera

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/bluetooth

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/ipc

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

NOTE: these were all done assuming we didn't have the static assertions to rely on, so they convert DispatchToMainThread/Dispatch(new foo(...)) to DispatchToMainThread/Dispatch(do_AddRef(new foo(...))).  This can be left or it could easily be trimmed back to drop the AddRef.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

silly bzexport mixing "export a patch" with "steal the bug".  At most it should do it only for bugs owned by nobody

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/events

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/cache

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/filesystem

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/storage

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

FYI, I have patches for dom/media that I'll upload as soon as I make sure they compile

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/ipc

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/cache

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/gmp

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/webrtc

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media (mostly doesn't force do_AddRef() on DispatchToMainThread(new Foo(...)))

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/webspeech

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/webaudio Note: doesn't do_AddRef for DispatchToMainThread(new ...)

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/mediasource

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/systemservices

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/eme

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

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

Ok, all the patches I had have been dumped here.  Builds on linux and runs.  Try in progress.

Note one additional pattern:
 nsRefPtr<FooRunnable> foo = new FooRunnable(...);
 thread->Dispatch(foo, ...)
 return foo.forget(); // or otherwise mess with foo, or foo is an mFoo member, etc

->
 nsRefPtr<FooRunnable> foo = new FooRunnable(...);
 thread->Dispatch(do_AddRef(foo.get()), ...);
 return foo.forget();

Note also the compile-time blocking of DispatchToMainThread() xidorn did needs to be extended to Dispatch() (and all the overloads thereof)

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

Note: I have updated patches for many of these after some try runs (and no surprise, these have already been bit-rotted, and will be again).  I'll probably refresh all of these shortly

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8635935 [details] [diff] [review]
DispatchToCurrentThread

Handled now in blocking bug 1186745

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

Attachment: static assertions to block Dispatch with plain smartptr (WIP, doesn't cover Dispatch() yet)

re-exporting all patches

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

Attachment: SyncRunnable

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WrapRunnable changes

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

Attachment: media/webrtc changes

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Attachment: media/mtransport changes

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/asmjscache

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Apparent real bugs in dom/*

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Possible bugs in dom/*

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/workers

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/various

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/camera

Comment 50

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/bluetooth

Comment 51

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/ipc

Comment 52

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/events

Comment 53

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/cache

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/filesystem

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/storage

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/gmp

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/eme

Comment 58

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/webrtc

Comment 59

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media (mostly doesn't force do_AddRef() on DispatchToMainThread(new Foo(...)))

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/webspeech

Comment 61

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/webaudio Note: doesn't do_AddRef for DispatchToMainThread(new ...)

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media/mediasource

Comment 63

[Randell Jesup [:jesup] (needinfo me)]

Local mochitest runs no longer leak; I have some tries running larger sets of mochitests to verify.
Apply in the order in the bug, though very few are if any are directly dependent on other patches.

Comment 64

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 65

[Randell Jesup [:jesup] (needinfo me)]

Looks like https://treeherder.mozilla.org/#/jobs?repo=try&revision=f10a2fd5d4c7 does not leak, and includes the media/mtransport changes, but not any of the DOM changes.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d016b61b1ac which has through dom/various does leak

Comment 66

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?

Comment 67

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?

Comment 68

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?

Comment 69

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?

Comment 70

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?

Comment 71

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?

Comment 72

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 73

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.