Last Comment Bug 1287706 - Support "once" option for addEventListener
: Support "once" option for addEventListener
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: Unspecified Unspecified
P3 normal with 3 votes (vote)
: mozilla50
Assigned To: Xidorn Quan [:xidorn] (UTC+10)
:
: Andrew Overholt [:overholt]
Mentors:
https://dom.spec.whatwg.org/#dictdef-...
: 1267505 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-19 00:49 PDT by Xidorn Quan [:xidorn] (UTC+10)
Modified: 2016-08-25 01:02 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
wip patch (5.46 KB, patch)
2016-07-20 00:13 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: feedback+
Details | Diff | Splinter Review
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only). (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Review
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener. (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Review
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable. (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Review
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events. (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Review
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function. (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
nfroyd: review+
Details | Review
Bug 1287706 part 6 - Separate notifying listener removal to an independent method. (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Review
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once. (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Review
Bug 1287706 part 8 - Add web-platform-test for once option. (58 bytes, text/x-review-board-request)
2016-07-25 20:03 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Review

Description User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-19 00:49:54 PDT
The DOM Standard added an "once" option for addEventListener, which would be quite useful, at least for test writing.

This option already exists in the WebIDL definition of AddEventListenerOptions, but the code to remove listener after its first use is not implemented.
Comment 1 User image Ben Kelly [not reviewing due to deadline][:bkelly] 2016-07-19 08:03:27 PDT
> This option already exists in the WebIDL definition of AddEventListenerOptions, but the code to remove listener after its first use is not implemented.

Does this mean people might feature detect that we support it when we actually don't?
Comment 2 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-19 08:05:00 PDT
See https://bugzilla.mozilla.org/show_bug.cgi?id=1266194#c5
which was just fixed in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1287760
Comment 3 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-20 00:13:36 PDT
Created attachment 8772713 [details] [diff] [review]
wip patch

Not sure whether this is the right way to go... If it is, I'll write a web-platform test and then request review.
Comment 4 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-20 03:43:59 PDT
Comment on attachment 8772713 [details] [diff] [review]
wip patch

># HG changeset patch
># User Xidorn Quan <me@upsuper.org>
># Date 1468998570 -36000
>#      Wed Jul 20 17:09:30 2016 +1000
># Node ID bc3d031a335a2bbdb494e66895f45130f65ec074
># Parent  97eb0e11fceb0ebe7513a2926781835dd2628bb5
>Bug 1287706 WIP - Support AddEventListenerOption.once in event code.
>
>MozReview-Commit-ID: 3ulIRxdApr6
>
>diff --git a/dom/events/EventListenerManager.cpp b/dom/events/EventListenerManager.cpp
>--- a/dom/events/EventListenerManager.cpp
>+++ b/dom/events/EventListenerManager.cpp
>@@ -1279,26 +1279,37 @@ EventListenerManager::HandleEventInterna
>                   (*aDOMEvent)->GetEventPhase(&phase);
>                   timelines->AddMarkerForDocShell(docShell, Move(
>                     MakeUnique<EventTimelineMarker>(
>                       typeStr, phase, MarkerTracingType::START)));
>                 }
>               }
>             }
> 
>+            Maybe<ListenerBase> listenerToRemove;
I don't understand why ListenerBase and not Listener

>+            if (listener->mFlags.mOnce) {
>+              listenerToRemove.emplace(*listener);
>+            }
>             aEvent->mFlags.mInPassiveListener = listener->mFlags.mPassive;
>             if (NS_FAILED(HandleEventSubType(listener, *aDOMEvent, aCurrentTarget))) {
>               aEvent->mFlags.mExceptionWasRaised = true;
>             }
>             aEvent->mFlags.mInPassiveListener = false;
> 
>             if (needsEndEventMarker) {
>               timelines->AddMarkerForDocShell(
>                 docShell, "DOMEvent", MarkerTracingType::END);
>             }
>+            if (listenerToRemove.isSome()) {
>+              RemoveEventListenerInternal(listenerToRemove->mListener,
>+                                          listenerToRemove->mEventMessage,
>+                                          listenerToRemove->mTypeAtom,
>+                                          listenerToRemove->mTypeString,
>+                                          listenerToRemove->mFlags, false);
>+            }
You need to remove the listener before the listener is called, otherwise nested event dispatch can use the listener again.
So, you need to keep the listener alive on stack, remove the listener from ELM and then call HandleEventSubType.
Note, the spec even says that listener should be removed before the listener is called.


>-  struct Listener
>+  struct ListenerBase
>   {
>     EventListenerHolder mListener;
>     nsCOMPtr<nsIAtom> mTypeAtom; // for the main thread
>     nsString mTypeString; // for non-main-threads
>     EventMessage mEventMessage;
>+    EventListenerFlags mFlags;
>+  };
> 
>+  struct Listener : public ListenerBase
Yeah, don't understand this ListenerBase stuff.


But yes, pretty much like this.
The test should definitely test nested dispatching.
Comment 5 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-20 04:44:07 PDT
I'm a bit worrying about the time complexity here... It is basically a O(N) deletion, so if someone attaches 1000 event listeners with once, that event dispatch would last for seconds... But I guess it isn't something we really need to worry about for the initial implementation?
Comment 6 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-20 04:47:19 PDT
(In reply to Olli Pettay [:smaug] from comment #4)
> >
> >diff --git a/dom/events/EventListenerManager.cpp b/dom/events/EventListenerManager.cpp
> >--- a/dom/events/EventListenerManager.cpp
> >+++ b/dom/events/EventListenerManager.cpp
> >@@ -1279,26 +1279,37 @@ EventListenerManager::HandleEventInterna
> >                   (*aDOMEvent)->GetEventPhase(&phase);
> >                   timelines->AddMarkerForDocShell(docShell, Move(
> >                     MakeUnique<EventTimelineMarker>(
> >                       typeStr, phase, MarkerTracingType::START)));
> >                 }
> >               }
> >             }
> > 
> >+            Maybe<ListenerBase> listenerToRemove;
> I don't understand why ListenerBase and not Listener

Because Listener has a non-trivial destructor which I don't want to call by mistake...
Comment 7 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-20 05:33:28 PDT
(In reply to Xidorn Quan [:xidorn] (away until July 25) from comment #5)
> I'm a bit worrying about the time complexity here... It is basically a O(N)
> deletion, so if someone attaches 1000 event listeners with once, that event
> dispatch would last for seconds... 
Well, removal time doesn't affect to that O(N)

> But I guess it isn't something we really
> need to worry about for the initial implementation?
Another option is to mark listener 'removed' and after iterating listeners for callback, iterate them again (if some was removed) and remove the marked ones. 'removed' flag should be check in places like AddEventListener and HasListenerFor etc. Becomes a tad complicated.

Though, I guess simpler would be to take a copy of the once-listener,  clear all the member variables of the listener in the array, then call the callback.
Then after iterating all the listener for the event, if there were once-listeners, compact the array by taking away the cleared listeners. That might not be to hard.
I guess compacting could be to create a new array and copy/move the real Listener entries from the old one to there and then swap the array with the old one.
Comment 8 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-20 19:04:19 PDT
(In reply to Olli Pettay [:smaug] from comment #7)
> (In reply to Xidorn Quan [:xidorn] (away until July 25) from comment #5)
> > But I guess it isn't something we really
> > need to worry about for the initial implementation?
> Another option is to mark listener 'removed' and after iterating listeners
> for callback, iterate them again (if some was removed) and remove the marked
> ones. 'removed' flag should be check in places like AddEventListener and
> HasListenerFor etc. Becomes a tad complicated.

Maintaining the array is simple. What I'm concerned is all the callbacks invoked in RemoveEventListenerInternal.
Comment 9 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-21 04:29:53 PDT
Things like NotifyAboutMainThreadListenerChange (which is mainly for devtools)? Well, that needs to be fired for all the listeners.
Comment 10 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

So that we can avoid unnecessary refcount changes.

Review commit: https://reviewboard.mozilla.org/r/67000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67000/
Comment 11 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

Review commit: https://reviewboard.mozilla.org/r/67002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67002/
Comment 12 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

Review commit: https://reviewboard.mozilla.org/r/67004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67004/
Comment 13 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

Review commit: https://reviewboard.mozilla.org/r/67006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67006/
Comment 14 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review commit: https://reviewboard.mozilla.org/r/67008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67008/
Comment 15 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review commit: https://reviewboard.mozilla.org/r/67010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67010/
Comment 16 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review commit: https://reviewboard.mozilla.org/r/67012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67012/
Comment 17 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 20:03:43 PDT
Created attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review commit: https://reviewboard.mozilla.org/r/67014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67014/
Comment 18 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 21:33:14 PDT
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67008/diff/1-2/
Comment 19 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 21:33:14 PDT
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67010/diff/1-2/
Comment 20 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 21:33:14 PDT
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67012/diff/1-2/
Comment 21 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-25 21:33:14 PDT
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67014/diff/1-2/
Comment 22 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 00:29:03 PDT
https://reviewboard.mozilla.org/r/67000/#review63920

::: dom/bindings/CallbackObject.h:519
(Diff revision 1)
> -    // NS_IF_RELEASE because we might have been unlinked before
> +    // We might have been unlinked before
> +    if (mPtrBits) {
> -    nsISupports* ptr = GetISupports();
> +      nsISupports* ptr = GetISupports();
> -    NS_IF_RELEASE(ptr);
> +      NS_RELEASE(ptr);
> -    mPtrBits = 0;
> +      mPtrBits = 0;
> -  }
> +    }

This change needs to be removed... It actually breaks things...
Comment 23 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67000/diff/1-2/
Comment 24 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67002/diff/1-2/
Comment 25 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67004/diff/1-2/
Comment 26 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67006/diff/1-2/
Comment 27 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67008/diff/2-3/
Comment 28 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67010/diff/2-3/
Comment 29 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67012/diff/2-3/
Comment 30 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 03:04:05 PDT
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67014/diff/2-3/
Comment 31 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 08:33:45 PDT
Comment on attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

https://reviewboard.mozilla.org/r/67002/#review64034
Comment 32 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 08:46:32 PDT
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

https://reviewboard.mozilla.org/r/67000/#review64036

Since I have no idea why we'd want this, r-
Comment 33 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 08:47:15 PDT
https://reviewboard.mozilla.org/r/67000/#review64038

feel free to explain why this improves code maintenance and makes writing code less error prone and then ask review again.
Comment 34 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 08:47:43 PDT
I wish I understood all the move stuff. Why is that needed? Move in general doesn't make code easier to read.
And why passing stuff by value and not by ref? What guarantees we aren't creating extra objects when using.
And in some cases Move(foo) is passed to ctor, in some cases Foo(param).
What happens in case one forgets to use Move()? Don't we create extra objects.
Comment 35 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 08:52:48 PDT
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

https://reviewboard.mozilla.org/r/67004/#review64044

::: dom/events/EventListenerManager.cpp:672
(Diff revision 2)
>  {
>    MOZ_ASSERT(aEventMessage == aEvent->mMessage ||
>               aEventMessage == GetLegacyEventMessage(aEvent->mMessage),
>               "aEvent and aEventMessage should agree, modulo legacyness");
>  
> +  // The listener has been removed, it cannot handle anything.

You need similar check to AddEventListener so that adding the listener again works.
Comment 36 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 11:30:48 PDT
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

https://reviewboard.mozilla.org/r/67014/#review64140
Comment 37 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 11:41:25 PDT
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

https://reviewboard.mozilla.org/r/67012/#review64144

r- because I don't think this stuff should use Move, given that nothing is really moved, mostly just cloned and then the original Listener's state is changed.

::: dom/events/EventListenerManager.cpp:1264
(Diff revision 3)
> +            Maybe<Listener> listenerHolder;
> +            if (listener->mFlags.mOnce) {
> +              // Move the listener to the stack before handling the event.
> +              // The order is important, otherwise the listener could be
> +              // called again inside the listener.
> +              listenerHolder.emplace(Move(*listener));

I don't understand the logic here.
listener is something which is stored in the array. How can you "Move" it? Move usage makes this hard to understand IMO, since nothing is really moved here.
Comment 38 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 11:46:23 PDT
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> Created attachment 8774578 [details]
> Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and
> nsTObserverArray to allow filtering elements by predicate function.
FWIW, I like this approach.
Comment 39 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 11:50:12 PDT
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

https://reviewboard.mozilla.org/r/67010/#review64156

::: dom/events/EventListenerManager.cpp:609
(Diff revision 3)
>        break;
>    }
>  }
>  
>  void
> +EventListenerManager::NotifyEventListenerRemoved(nsIAtom* aUserType)

Please add a comment that there are also other cases when listener can be removed and notified.

I wonder if EventListenerManager::RemoveEventHandler could use this new method.
Comment 40 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-26 12:27:16 PDT
Comment on attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

https://reviewboard.mozilla.org/r/67006/#review64178
Comment 41 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 16:49:57 PDT
(In reply to Olli Pettay [:smaug] from comment #34)
> I wish I understood all the move stuff. Why is that needed? Move in general
> doesn't make code easier to read.

Move is a concept that moving the ownership of resource from one instance to another, and reset the original instance to its initial state or leave it in an (sounded) unusable state. For example, if our nsCOMPtr has a move constructor (which it doesn't yet), then
> nsCOMPtr somePtr(Move(anotherPtr));
would just be identical to
> nsCOMPtr somePtr(anotherPtr.forget());

And if nsTArray has a move construct (which it doesn't yet, either), then
> nsTArray<Foo> foos(Move(bars));
should be just set the foos.mHdr to bars.mHdr, and make bars.mHdr refer to the empty hdr.

> And why passing stuff by value and not by ref? What guarantees we aren't
> creating extra objects when using.

Passing CallbackObjectHolder by value because it is just a pointer-length type, which is not more expensive than a reference (which is a pointer as well), and passing by value potentially gives compilers more chance to optimize because compilers know that the original variable no longer owns the resource anyway. (If it is passed by reference, compilers cannot guarantee that if the callee is opaque in this translation unit.)

How could extra object be created? CallbackObjectHolder does not create object at all. In our current code, we use copy constructor, which means every time you pass it in, you have an extra AddRef / Release on the held object. However, with move, the old CallbackObjectHolder would just hand over its ownership to the new one, and no AddRef / Release happens.

Let's use a simplified RefPtr as an example (which is almost the same as CallbackObjectHolder), its copy constructor would be something like:
> RefPtr(const RefPtr& aOther) : mPtr(aOther.mPtr) { NS_IF_ADDREF(mPtr); }
there is an AddRef. And if a class needs that pointer, and we have
> Foo(const RefPtr& aPtr) : mPtr(aPtr) {}
and at the callsite
> { RefPtr ptr = something; Foo(ptr); }
we would call the destructor on ptr at the end of the scope, which calls NS_IF_RELEASE to the object it holds. However, this pair of AddRef / Release is not needed at all.

So in the move case, what would happen is, the move constructor would be something like
> RefPtr(RefPtr&& aOther) : mPtr(aOther.mPtr) { aOther.mPtr = nullptr; }
no AddRef here, aOther is just reset to nullptr. And the class constructor would be
> Foo(RefPtr aPtr) : mPtr(Move(aPtr)) {}
and at the callsite
> { RefPtr ptr = something; Foo(Move(ptr)); }
For the class constructor, the move constructor would be used to initialize aPtr from the callsite ptr, so no AddRef, and mPtr would use the move constructor as well, so no AddRef. All the initialization is just copying a pointer-length value from one place to another. And at the end of the callsite scope, ptr is reset to nullptr, so no Release there. And given that the compiler knows ptr is guaranteed to be reset to nullptr, it may just remove the destructor call completely.

In addition, as far as we make the object move-only (mark the copy constructor deleted), we would never AddRef / Release the held object when moving it between variables by accident.

> And in some cases Move(foo) is passed to ctor, in some cases Foo(param).
> What happens in case one forgets to use Move()? Don't we create extra
> objects.

If we forget Move() for Move(foo):
* if Foo is move-only, than the compiling would fail, because it cannot find a constructor for it;
* if Foo is copyable, then the copy constructor would be used.

A constructor accepts a rvalue reference (Foo&&) is called a move constructor, and it is expected to transfer the ownership from the parameter to itself (as showed above). A type which has move constructor but no copy constructor is a move-only type.

Roughly speaking, a lvalue reference (Foo&) is not implicitly-convertible to a rvalue reference (Foo&&), and thus would not be accepted by a move constructor. Expressions return a lvalue reference usually include forms of `foo`, `*foo`, and `foos[i]`. So if Foo is move-only, compiler would simply reject Foo(foo). In that case, Move() is used to convert the type from Foo& to Foo&& to explicitly state that we do want to move the ownership of the held resource from one variable to somewhere else.

And expression returns Foo would have type Foo&&, because the return value of an expression is just a temporary value, which is not bound to any variable, and thus can be moved freely. And that was the reason why Foo(param) can be used, because it is an expression creating a temporary value, which would be moved to the parameter.

FWIW, Foo&& can be implicitly converted to `const Foo&`, and thus if a type lacks move constructor, it would fallback to a copy constructor.


Is the explanation clear enough to you?
Comment 42 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 16:53:25 PDT
https://reviewboard.mozilla.org/r/67004/#review64044

> You need similar check to AddEventListener so that adding the listener again works.

Actually it doesn't, because the move constructor of Listener resets all the fields (especially mListener), so the listener can surely be added again.
Comment 43 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 16:55:10 PDT
https://reviewboard.mozilla.org/r/67012/#review64144

> I don't understand the logic here.
> listener is something which is stored in the array. How can you "Move" it? Move usage makes this hard to understand IMO, since nothing is really moved here.

If "move" all the resource it holds to the stack, and leave the original place an unusable placeholder (which ideally would be skipped everywhere).
Comment 44 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 16:55:32 PDT
https://reviewboard.mozilla.org/r/67012/#review64144

> If "move" all the resource it holds to the stack, and leave the original place an unusable placeholder (which ideally would be skipped everywhere).

I mean, it "move"s all the resources.
Comment 45 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:42:03 PDT
https://reviewboard.mozilla.org/r/67010/#review64156

> Please add a comment that there are also other cases when listener can be removed and notified.
> 
> I wonder if EventListenerManager::RemoveEventHandler could use this new method.

I don't really understand what do you want me to add. Is that about listing the callsites of this function? I don't think that's worth a comment.

And it seems yes, EventListenerManager::RemoveEventHandler can use this new method. I'll update the patch and use it there.
Comment 46 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67000/diff/2-3/
Comment 47 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67002/diff/2-3/
Comment 48 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67004/diff/2-3/
Comment 49 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67006/diff/2-3/
Comment 50 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67008/diff/3-4/
Comment 51 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67010/diff/3-4/
Comment 52 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67012/diff/3-4/
Comment 53 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-26 20:46:06 PDT
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67014/diff/3-4/
Comment 54 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-27 08:27:36 PDT
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #41)
> (In reply to Olli Pettay [:smaug] from comment #34)
> > I wish I understood all the move stuff. Why is that needed? Move in general
> > doesn't make code easier to read.
> 
> Move is a concept that moving the ownership of resource from one instance to
> another, and reset the original instance to its initial state or leave it in
> an (sounded) unusable state.
Sure, but the cases here aren't quite like that.


> > And why passing stuff by value and not by ref? What guarantees we aren't
> > creating extra objects when using.
> 
> Passing CallbackObjectHolder by value because it is just a pointer-length
> type, which is not more expensive than a reference (which is a pointer as
> well), and passing by value potentially gives compilers more chance to
> optimize because compilers know that the original variable no longer owns
> the resource anyway. (If it is passed by reference, compilers cannot
> guarantee that if the callee is opaque in this translation unit.)
What if more member variables are added to CallbackObjectHolder? 
Very unusual to pass by value.

> How could extra object be created? CallbackObjectHolder does not create
> object at all.
What you mean? If you create CallbackObjectHolder object, you sure create one :)
nsIDocument::CreateTreeWalker(*root, aWhatToShow, NodeFilterHolder(aFilter), rv)
creates a NodeFilterHolder object, and then one the callee side the object is created as well
(&& ctor is invoked). Sure compiler may or may not optimize the extra object out.


> > RefPtr(RefPtr&& aOther) : mPtr(aOther.mPtr) { aOther.mPtr = nullptr; }
> no AddRef here, aOther is just reset to nullptr. And the class constructor
> would be
> > Foo(RefPtr aPtr) : mPtr(Move(aPtr)) {}
This kind of ctor on Foo would be a bug IMO, since it let's one to pass lvalues, in which case extra addref/release could happen. 


> If we forget Move() for Move(foo):
> * if Foo is move-only, than the compiling would fail, because it cannot find
> a constructor for it;
> * if Foo is copyable, then the copy constructor would be used.
Yes. That is surprising and a bug, IMO.

> Is the explanation clear enough to you?
Not really. Doesn't explain why you want to change this all to use move semantics, especially in this bug.
Comment 55 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-27 08:44:22 PDT
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

https://reviewboard.mozilla.org/r/67004/#review64146

If default and move ctors leave the object to the same state, r+

::: dom/events/EventListenerManager.h:225
(Diff revision 2)
> +      , mAllEvents(aOther.mAllEvents)
> +      , mIsChrome(aOther.mIsChrome)
> +    {
> +      aOther.mTypeString.Truncate();
> +      aOther.mEventMessage = eVoidEvent;
> +      aOther.mListenerType = eNoListener;



::: dom/events/EventListenerManager.h:225
(Diff revision 3)
> +      , mAllEvents(aOther.mAllEvents)
> +      , mIsChrome(aOther.mIsChrome)
> +    {
> +      aOther.mTypeString.Truncate();
> +      aOther.mEventMessage = eVoidEvent;
> +      aOther.mListenerType = eNoListener;

This is really surprising that only when Move ctor is used, the other object suddenly gets eNoListener status which is never ever otherwise would have.
This gives 'move' quite a new semantics:
"move and change state", instead of the normal 
"move and reset".

I could live with this setup if the default ctor would explicitly initialize values similarly to move ctor.
Comment 56 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-27 08:48:41 PDT
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #45)
> https://reviewboard.mozilla.org/r/67010/#review64156
> 
> > Please add a comment that there are also other cases when listener can be removed and notified.
> > 
> > I wonder if EventListenerManager::RemoveEventHandler could use this new method.
> 
> I don't really understand what do you want me to add. Is that about listing
> the callsites of this function? I don't think that's worth a comment.
No. I mean that listener being removed is notified also elsewhere. The new method doesn't cover all the cases.
Comment 57 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-27 10:18:23 PDT
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

https://reviewboard.mozilla.org/r/67000/#review64508

ok, I guess I can live with this, but please add that comment to CallbackObjectHolder ctor.

::: dom/bindings/CallbackObject.h:427
(Diff revision 3)
>    explicit operator bool() const
>    {
>      return GetISupports();
>    }
>  
> +  CallbackObjectHolder Clone() const

gah, I hate C++'s move. It is so broken concept.
Ok, here we don't cause extra addref/release, since result ends up being assigned to another instance of CallbackObjectHolder using move ctor.

::: dom/events/EventListenerManager.cpp:303
(Diff revision 3)
>    } else if ((wjs = do_QueryInterface(aListenerHolder.GetXPCOMCallback()))) {
>      listener->mListenerType = Listener::eWrappedJSListener;
>    } else {
>      listener->mListenerType = Listener::eNativeListener;
>    }
> +  listener->mListener = Move(aListenerHolder);

Hmm, so first EventListener holder is moved to a temporary object which is then used to construct the argument for AddEventListener and that is then moved to create a temporary object which is used to create the argument for AddEventListenerByType which is then moved to create temporary object for AddEventListenerInternal to create the argument for it which is then moved (if the listener isn't already registered) to listener->mListener.
Compiler may of course optimize out many steps and in this case given that CallbackHolder is simple, it should be all fast. But this is a bit hazard to any possible changes to CallbackObjectHolder.
In case anything more complicated was passed, & or * param would probably be faster.
So, please add a comment to
CallbackObjectHolder(CallbackObjectHolder&& aOther) that it must not do anything complicated, or the setup should be changed to use pointer or reference passing.
Comment 58 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-27 10:31:29 PDT
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

https://reviewboard.mozilla.org/r/67012/#review64520

ok, move then. (Swap would be so much more readable.)
Comment 59 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-27 10:40:30 PDT
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

https://reviewboard.mozilla.org/r/67010/#review64522

::: dom/events/EventListenerManager.cpp:610
(Diff revision 4)
>    }
>  }
>  
>  void
> +EventListenerManager::NotifyEventListenerRemoved(nsIAtom* aUserType)
> +{

So somewhere here mention that if this code is changed, other callers of EventListenerRemoved and NotifyAboutMainThreadListenerChange should be changed too.
It is just that the name NotifyEventListenerRemoved hints that this is the way to notify about changes, but there are also other cases.
Comment 60 User image Nathan Froyd [:froydnj] 2016-07-27 11:09:22 PDT
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

https://reviewboard.mozilla.org/r/67008/#review64530

::: xpcom/glue/nsTArray.h:1598
(Diff revision 3)
> +  // This method removes elements based on the return value of the
> +  // callback function aPredicate. If the function returns true for
> +  // an element, the element is removed. aPredicate will be called
> +  // for each element in order. It is not safe to access the array
> +  // inside aPredicate.
> +  void RemoveElementsBy(mozilla::function<bool(const elem_type&)> aPredicate);

We are going to have to make mozilla::function allocation-free at some point if we're going to start adding a bunch of interfaces like this.

::: xpcom/glue/nsTArray.h:1919
(Diff revision 3)
> +        copy_type::MoveNonOverlappingRegion(Elements() + j, Elements() + i,
> +                                            1, sizeof(elem_type));

It would be kind of nice to do block copies where possible, but I think keeping track of that would significantly complicate the implementation.
Comment 61 User image Xidorn Quan [:xidorn] (UTC+10) 2016-07-27 16:53:02 PDT
https://reviewboard.mozilla.org/r/67008/#review64530

> We are going to have to make mozilla::function allocation-free at some point if we're going to start adding a bunch of interfaces like this.

It's supprising that mozilla::function needs allocation...

> It would be kind of nice to do block copies where possible, but I think keeping track of that would significantly complicate the implementation.

For the only currently use, block copy wouldn't make anything better (because we need to call ctor and dtor for each element). I considered doing block copy when I wrote this, but I thought that is a bit too complicated, and I'd like to start with a simple one. But yeah, we should definitely consider makeing it do block copies later. I'll file a bug.
Comment 62 User image Pulsebot 2016-07-27 19:01:31 PDT
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/003da22902a6
part 1 - Make CallbackObjectHolder movable (and actually move-only). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee0b27b208d1
part 2 - Remove useless runtime check of EventListenerManager::Listener. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b858cd9f95fb
part 3 - Make EventListenerHandler::Listener movable. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26a98e3e7b0
part 4 - Unify B2G device-type events to be handled in the same way as others device-type events. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a53cb28a89a1
part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/2439dff9e9c4
part 6 - Separate notifying listener removal to an independent method. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5c362afc7c
part 7 - Add support of AddEventListenerOptions.once. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b876fe012a
part 8 - Add web-platform-test for once option. r=smaug
Comment 65 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-08-19 07:03:14 PDT
*** Bug 1267505 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.