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

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed)

Details

Attachments

(3 attachments)

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.
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.
(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.
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.
Attachment #769458 - Flags: review?(gkrizsanits)
Attachment #769458 - Flags: review?(gkrizsanits) → review+
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?
Attachment #769459 - Flags: review?(gkrizsanits) → review+
Attachment #769460 - Flags: review?(gkrizsanits) → review+
Blocks: 860085
You need to log in before you can comment on or make changes to this bug.