910156 - Some new members for D3E in ns*Event are not copied in nsDelayed*Event

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I realized that nsDelayed*Event is now copying ns*Event manually. D3E changes don't change here.

Comment 1

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

ah, good catch. Can we make ns*Event to have such method or copy constructor that manual member
variable copying wouldn't be necessary.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, I'm creating Assign*EventData() methods which are not virtual methods.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=405c1430f3ed
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d3007946cb93

Only Android R5 is orange, but I guess this is not this patch. I'll land this patch after I confirmed it later.

This patch adds Assign*EventData() to nsEvent, nsGUIEvent, nsKeyEvent, nsMouseEvent_base and nsMouseEvent for now.

nsDOMEvent::DuplicatePrivateData() and nsDelayed*Event share the methods. So, developers will not forget to copy new member in the future!

Assign*EventData() takes |bool aCopyRefPointer| for its second argument. When it's false, the methods don't copy nsCOMPtr<> and nsRefPtr<> members. So, if it specifies false, the copied instance don't grab other objects. I don't like this argument, though. But I think that this is the simplest way. I think only one method should be added to ns*Event and caller shouldn't clear the pointers after assign. The reason is, if callers clear them manually, developers will do similar mistake when they add nsCOMPtr<> or nsRefPtr<>.

The new automated test tests only check nsKeyEvent and nsMouseEvent for now. I'll file a follow up bug for the other events.

Additionally, I find a bug. nsDOMKeyEvent don't return correct .key value because mEventIsInternal becomes true at the end of nsDOMEvent::DuplicatePrivateData(). I'll file a new bug for this too.

Finally, if default action consumes the event, then, the stored event's defaultPrevented becomes true. I'm not sure whether this is correct behavior or not. If it's not so, I think that event dispatcher should cache the defaultPrevented value before dispatching an event to system group and restore the value after that.

Comment 4

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

Comment on attachment 797161 [details] [diff] [review]
Patch

>+      nsGUIEvent* GUIEvent = new nsGUIEvent(false, msg, nullptr);
Perhaps nsGUIEvent* guiEvent;
Odd to see local variables to start with capital letter.
Same also elsewhere.

>+  void AssignEventData(const nsEvent& aEvent, bool aCopyRefPointer)
>+  {
>+    // eventStructType, message should be initialized with the constructor.
>+    refPoint = aEvent.refPoint;
>+    // lastRefPoint doesn't need to be copied.
>+    time = aEvent.time;
>+    // mFlags should be copied manually if it's necessary.
>+    userType = aCopyRefPointer ? aEvent.userType : nullptr;
aCopyRefPointer is indeed odd if it means also copying userType for example.


>+    // typeString should be copied manually if it's necessary.
>+    target = aCopyRefPointer ? aEvent.target : nullptr;
>+    currentTarget = aCopyRefPointer ? aEvent.currentTarget : nullptr;
>+    originalTarget = aCopyRefPointer ? aEvent.originalTarget : nullptr;
And target.

Couldn't we have bool aCopyTargets. Copying userType shouldn't cause harm.



>+function doTest(aTest)
>+{
>+  if (!aTest.canRun()) {
>+    SimpleTest.executeSoon(runNextTest);
>+    return;
>+  }
>+  gEvent = null;
>+  gCopiedEvent = [];
>+  var target = document.getElementById(aTest.targetID);
>+  target.addEventListener(aTest.eventType, onEvent, true);
>+  setTimeout(function () {
>+    target.removeEventListener(aTest.eventType, onEvent, true);
>+    ok(gEvent !== null, aTest.description + ": failed to get duplicated event");
>+    ok(gCopiedEvent.length > 0, aTest.description + ": count of attribute of the event must be larger than 0");
>+    for (var i = 0; i < gCopiedEvent.length; ++i) {
>+      var name = gCopiedEvent[i].name;
>+      if (name == "rangeOffset") {
>+        todo(false, aTest.description + ": " + name + " attribute value is never reset (" + gEvent[name] + ")");
>+      } else if (name == "eventPhase") {
>+        is(gEvent[name], 0, aTest.description + ": mismatch with fixed value (" + name + ")");
>+      } else if (name == "rangeParent" || name == "currentTarget") {
>+        is(gEvent[name], null, aTest.description + ": mismatch with fixed value (" + name + ")");
>+      } else if (aTest.todoMismatch.indexOf(name) >= 0) {
>+        todo_is(gEvent[name], gCopiedEvent[i].value, aTest.description + ": mismatch (" + name + ")");
>+      } else {
>+        is(gEvent[name], gCopiedEvent[i].value, aTest.description + ": mismatch (" + name + ")");
>+      }
>+    }
>+    runNextTest();
>+  }, 100);
>+  aTest.dispatchEvent();
>+}
I don't understand the setTimeout(..., 100). There shouldn't be need for that.
setTimeout(..., 0) should be enough.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for your quick review!

I'm check the orange again:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d184664ce6d1

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The R5 on B2G orange must be an issue of tryserver. Other people's R5s on B2G are also orange with same log.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff6833625a2

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9ff6833625a2