888104 - Reimplement nsCxPusher in terms of a more well-behaved RAII class

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The nsCxPusher API is pretty wonky. There are a number of warts, but the most egregious is RePush(), which allows internal state to be torn down and the pusher to be re-used. This gets really tricky when adding more RAII functionality to nsCxPusher, because everything has to know how to tear itself down.

The better solution is to make a simpler, constructor/destructor-bound class, and use Maybe<>s to do all the funny stuff. To avoid changing the ~50 consumers of nsCxPusher, I'm going to make that class a wrapper around the new one.

Comment 1

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

Note, RePush was added for performance reasons. It helps with event handling *a lot* - well at least
helped when it was added. Haven't profiled that part lately.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Note, RePush was added for performance reasons. It helps with event handling
> *a lot* - well at least
> helped when it was added. Haven't profiled that part lately.

Presumably the performance improvement came from the fact that RePush skips the push if the old scx matches the new one. I've kept that optimization, so we should be ok here.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Move nsCxPusher's mScx grabbing code into the common helper method. v1

This function is called by Push and PushNull, so with the added null check this
is equivalent. This facilitates the refactoring in the next patch.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Introduce AutoCxPusher and reimplement nsCxPusher in terms of it. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Reimplement Auto*JSContext in terms of AutoCxPusher. v1

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 769459 [details] [diff] [review]
Part 2 - Introduce AutoCxPusher and reimplement nsCxPusher in terms of it. v1

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

+using namespace mozilla;

It's nor really clear why you need this. AutoCxPusher is in this namespace but for other class in the same file we have: 

 namespace mozilla {
 
 AutoJSContext::AutoJSContext(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL)


 bool
 nsCxPusher::Push(EventTarget *aCurrentTarget)
 {
-  if (mPushedSomething) {
-    NS_ERROR("Whaaa! No double pushing with nsCxPusher::Push()!");
-
-    return false;
-  }
-

I think we should at least keep a mPusher.empty() assert here, no?

+  nsIScriptContext* GetCurrentScriptContext() {
+    return mPusher.ref().GetScriptContext();
+  }

I understand that this is legacy and used only from one place and all, but still a getter that can crash is a bit whacky. Can we do an mPusher.empty() check here?

Comment 7

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df08a8c32fcc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7caba9766f5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f4163dd261d

Comment 8

[Bobby Holley (:bholley)]

Followup fix for windows bustage:

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df08a8c32fcc
https://hg.mozilla.org/mozilla-central/rev/7caba9766f5f
https://hg.mozilla.org/mozilla-central/rev/0f4163dd261d
https://hg.mozilla.org/mozilla-central/rev/ea16f566fd50

Comment 10

[Bobby Holley (:bholley)]

Uplifted to 24 as a dep of bug 860085:

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=dd08f5dd367c