1171785 - create nsContentUtils RunInStableState helper

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Karl Tomlinson (:karlt)]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1116382#c18

> +#include "nsWidgetsCID.h"
> +#include "nsIAppShell.h"

> We really need to stop including all this widget junk everywhere just to do
> RunInStableState. Can you make a helper on nsContentUtils to abstract away the > appshell piece? Followup is fine.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: create nsContentUtils::RunInStableState helper

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: use nsContentUtils::RunInStableState()

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8615789 [details] [diff] [review]
create nsContentUtils::RunInStableState helper

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

::: dom/base/nsContentUtils.h
@@ +1581,5 @@
> +   * In practice this runs aRunnable once the currently executing event
> +   * finishes. If called multiple times per task/event, all the runnables will
> +   * be executed, in the order in which RunInStableState() was called.
> +   */
> +  static nsresult RunInStableState(nsIRunnable* aRunnable);

Please mention that this runnable must not spin the event loop.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8615789 [details] [diff] [review]
create nsContentUtils::RunInStableState helper

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

Actually, can you just make this infallible and assert that it succeeds? We're moving APIs towards infallible dispatch, since there's rarely anything useful to do when it fails.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8615789 [details] [diff] [review]
create nsContentUtils::RunInStableState helper

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

Oh, and one more thing - can you make this take an already_AddRefed, so that we can move our dispatch API usage in the direction of .forget() as well?

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8615790 [details] [diff] [review]
use nsContentUtils::RunInStableState()

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

Looks great, r=me modulo the API changes requested in part 1.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: create nsContentUtils::RunInStableState helper v2

Some changes are a little awkward, so I think another review is appropriate.

I like the API because I think of nsIRunnables as one-shot objects, but
nsIRunnables are not always used in this way.

I'm not sure whether this change should or should not depend on acceptance of
the equivalent change in nsIAppShell.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: use nsContentUtils::RunInStableState()

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8616515 [details] [diff] [review]
use nsContentUtils::RunInStableState()

We can't assume infallibility for AudioDestinationNode::ScheduleStableStateNotification() because this is triggered from AudioNode destruction, which may happen during cycle collector shutdown, which runs after mozilla::services::Shutdown();

https://hg.mozilla.org/mozilla-central/annotate/7d4ab4a9febd/dom/media/webaudio/AudioNode.cpp#l88

https://hg.mozilla.org/mozilla-central/annotate/7d4ab4a9febd/xpcom/build/XPCOMInit.cpp#l932

https://treeherder.mozilla.org/logviewer.html#?job_id=8299191&repo=try
Thread 0 (crashed)
0 libxul.so!nsContentUtils::RunInStableState(already_AddRefed<nsIRunnable>) [nsContentUtils.cpp:80c722c7218b : 5141 + 0x19]
eip = 0xb3339754 esp = 0xbfff80b0 ebp = 0xbfff80e8 ebx = 0xb7612ec0
esi = 0xbfff80c8 edi = 0xbfff811c eax = 0x00000000 ecx = 0xb250a8ac
edx = 0x00000000 efl = 0x00210286
Found by: given as instruction pointer in context
1 libxul.so!mozilla::dom::AudioDestinationNode::ScheduleStableStateNotification() [AudioDestinationNode.cpp:80c722c7218b : 691 + 0xe]
eip = 0xb3dc684d esp = 0xbfff80f0 ebp = 0xbfff8138 ebx = 0xb7612ec0
esi = 0xbfff8118 edi = 0xbfff811c
Found by: call frame info
2 libxul.so!mozilla::dom::AudioNode::~AudioNode() [AudioNode.cpp:80c722c7218b : 88 + 0x15]
eip = 0xb3dc7099 esp = 0xbfff8140 ebp = 0xbfff8168 ebx = 0xb7612ec0
esi = 0xa4e31430 edi = 0xbfff824c
Found by: call frame info
3 libxul.so!mozilla::dom::MediaStreamAudioSourceNode::~MediaStreamAudioSourceNode() [MediaStreamAudioSourceNode.cpp:80c722c7218b : 57 + 0x8]
eip = 0xb3dcd5e2 esp = 0xbfff8170 ebp = 0xbfff8188 ebx = 0xb7612ec0
esi = 0xa4e31430 edi = 0xbfff824c
Found by: call frame info
4 libxul.so!mozilla::DOMEventTargetHelper::DeleteCycleCollectable() [DOMEventTargetHelper.cpp:80c722c7218b : 76 + 0x8]
eip = 0xb3b9ac94 esp = 0xbfff8190 ebp = 0xbfff81a8 ebx = 0xb7612ec0
esi = 0xa4e31430 edi = 0xbfff824c
Found by: call frame info
5 libxul.so!mozilla::DOMEventTargetHelper::cycleCollection::DeleteCycleCollectable(void*) [DOMEventTargetHelper.h:80c722c7218b : 64 + 0x7]
eip = 0xb332f2ac esp = 0xbfff81b0 ebp = 0xbfff81c8 ebx = 0xb7612ec0
esi = 0xa4e31430 edi = 0xbfff824c
Found by: call frame info
6 libxul.so!SnowWhiteKiller::~SnowWhiteKiller() [nsCycleCollector.cpp:80c722c7218b : 2637 + 0x10]
eip = 0xb29f63cc esp = 0xbfff81d0 ebp = 0xbfff8218 ebx = 0xb7612ec0
esi = 0x8df49000 edi = 0xbfff824c
Found by: call frame info
7 libxul.so!nsCycleCollector::FreeSnowWhite(bool) [nsCycleCollector.cpp:80c722c7218b : 2804 + 0xb]
eip = 0xb29f64f4 esp = 0xbfff8220 ebp = 0xbfff8278 ebx = 0xb7612ec0
esi = 0xadc3a000 edi = 0x00000000
Found by: call frame info
8 libxul.so!nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) [nsCycleCollector.cpp:80c722c7218b : 3772 + 0x9]
eip = 0xb29f67a1 esp = 0xbfff8280 ebp = 0xbfff82d8 ebx = 0xb7612ec0
esi = 0xadc3a000 edi = 0x00000000
Found by: call frame info
9 libxul.so!nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [nsCycleCollector.cpp:80c722c7218b : 3597 + 0xf]
eip = 0xb29f72b7 esp = 0xbfff82e0 ebp = 0xbfff8318 ebx = 0xb7612ec0
esi = 0xbfff8344 edi = 0xadc3a000
Found by: call frame info
10 libxul.so!nsCycleCollector::ShutdownCollect() [nsCycleCollector.cpp:80c722c7218b : 3543 + 0x11]
eip = 0xb29f78d2 esp = 0xbfff8320 ebp = 0xbfff8378 ebx = 0xb7612ec0
esi = 0x00000000 edi = 0xbfff8344
Found by: call frame info
11 libxul.so!nsCycleCollector_shutdown() [nsCycleCollector.cpp:80c722c7218b : 4153 + 0xf]
eip = 0xb29f79c3 esp = 0xbfff8380 ebp = 0xbfff83b8 ebx = 0xb7612ec0
esi = 0xb0fff098 edi = 0xb76612f0
Found by: call frame info
12 libxul.so!mozilla::ShutdownXPCOM(nsIServiceManager*) [XPCOMInit.cpp:80c722c7218b : 932 + 0x4]
eip = 0xb2a68111 esp = 0xbfff83c0 ebp = 0xbfff8408 ebx = 0xb7612ec0
esi = 0xbfff83d8 edi = 0xb561ddb0

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8616514 [details] [diff] [review]
create nsContentUtils::RunInStableState helper v2

I think it is better to use the same helper everywhere and assert where it matters if the runnable is not scheduled.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #10)
> Comment on attachment 8616514 [details] [diff] [review]
> create nsContentUtils::RunInStableState helper v2
> 
> I think it is better to use the same helper everywhere and assert where it
> matters if the runnable is not scheduled.

I think we should do the same thing that I did with AbstractThread::Dispatch - have a default second param of type DispatchFailureHandling, and pass DontAssertDispatchSuccess where needed.

Having each caller decide between (a) manually asserting against dispatch failure, (b) trying to handle dispatch failure, or (c) ignoring it is really crappy. We're generally moving our dispatch stuff towards being infallible, so I think this is the right way to go.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: create nsContentUtils::RunInStableState helper v3

I proposed

  void RunInStableState(already_AddRefed<nsIRunnable> aRunnable,
                        nsresult* aErrorRv = nullptr)

where the second parameter behaves like a try/catch, allowing the caller to
choose how to act on failure, but that is not necessary at this stage, and
Bobby didn't sound convinced.

The API for NS_DispatchToMainThread hasn't yet been determined.

So going with DispatchFailureHandling to assert or ignore, for now, and we can
change the single ignore use case in the future if there is a need to change.

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

<karlt> bholley: thanks for the DispatchFailureHandling suggestion; i think that handles the existing use cases and can test with try; however, that doesn't provide a way to return indication of the failure, should a caller want to choose how to act on failure; what do you think about an optional nsresult out param?
<bholley> karlt: does the caller really have anything useful to do in the failure case?
<bholley> karlt: generally I think it doesn't
<karlt> bholley: i don't think existing callers have, though i have seen cases in MSG where the event is run sync
<karlt> cases == different dispatch mechanisms
<bholley> karlt: well, jib was the one who was trying to make dispatch infallible :-)
<bholley> karlt: my general feeling is that most of those last-ditch "let's run this sync" are a bad idea
<bholley> karlt: we've done that elsewhere in gecko with terrible results
<karlt> given this only fails after service shutdown, i think the only need to do something would be to clean up leaks
<karlt> msg has to run the sync stuff in the right way to avoid terrible results
<bholley> karlt: I mean, we can still return a failure code from the same method if we really want to
<bholley> rather than as an out-param
<karlt> i don't mind too much, but even if we didn't use it i thought the optional nsresult as a "try/catch" wasn't too bad an api
<bholley> karlt: but I'd still question any such usage
<bholley> karlt: my point is that we should make this look like what nsIThread dispatch is going to look like in the future
<karlt> bholley: has that been decided do you know?
<bholley> karlt: bug 1155059, I think?
<firebot> https://bugzil.la/1155059 — NEW, [e-mail] — Switch Dispatch() and friends to take already_AddRefed<nsIRunnable>'s
<jesup> Ironically I'm fighting that bug right now...
<karlt> also bug 803174 tried this
<firebot> https://bugzil.la/803174 — NEW, [e-mail] — Dispatch() should be infallible by default
<karlt>   and bug 1142799
<firebot> https://bugzil.la/1142799 — NEW, [e-mail] — Make NS_DispatchToMainThread infallible

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: use nsContentUtils::RunInStableState() v3

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8617663 [details] [diff] [review]
create nsContentUtils::RunInStableState helper v3

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

This looks great, thank you.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8617664 [details] [diff] [review]
use nsContentUtils::RunInStableState() v3

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

r=me with that change.

::: dom/html/HTMLImageElement.h
@@ +338,5 @@
>  
>    static void MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
>                                      nsRuleData* aData);
>  
> +  nsIRunnable* MOZ_NON_OWNING_REF mPendingImageLoadTask = nullptr;

Hm, this is a pretty scary change - I see that it looks correct now, but I'm not sure if that's really how we want to do it, since it make it easier to UAF.

Can you revert this change for this patch, and file a followup bug if you'd like to make it?

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecbb53da4e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b61b49cd8e9b

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Bobby Holley (:bholley) from comment #15)
> Can you revert this change for this patch, and file a followup bug if you'd
> like to make it?

I reverted that change.  I won't bother with a followup as its only an additional refcnt manipulation on what I expect is not a performance-critical path.

I also dropped the runnable/aRunnable parameter name change in nsIDOMWindowUtils.idl because the push script was complaining about not changing the uuid.  I guess I could have put some magic words in the commit script, but the change was not necessary.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ecbb53da4e0
https://hg.mozilla.org/mozilla-central/rev/b61b49cd8e9b