Change in gwt[master]: Fix module unloading with multiple modules on a page

[Matthew Dempsky]

Matthew Dempsky has uploaded a new change for review.

https://gwt-review.googlesource.com/2941


Change subject: Fix module unloading with multiple modules on a page
......................................................................

Fix module unloading with multiple modules on a page

Currently, the GWT compiler in production compiles will optimize away
the instanceof checks in isMyListener() because it assumes
getEventListener() is actually returning an EventListener object.
However, if there are multiple modules loaded on a page, the
__listener property might be for an EventListener from a different
module.

One way to workaround this (the approach taken by this patch) is to
call isMyListener() before getEventListener() returns, so the compiler
can't optimize away the instanceof checks.

Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
---
M user/src/com/google/gwt/user/client/impl/DOMImpl.java
1 file changed, 5 insertions(+), 6 deletions(-)



diff --git a/user/src/com/google/gwt/user/client/impl/DOMImpl.java
b/user/src/com/google/gwt/user/client/impl/DOMImpl.java
index c5b8187..eafd332 100644
--- a/user/src/com/google/gwt/user/client/impl/DOMImpl.java
+++ b/user/src/com/google/gwt/user/client/impl/DOMImpl.java
@@ -50,12 +50,9 @@
for (int i = 0; i < allElements.getLength(); i++) {
com.google.gwt.dom.client.Element elem = allElements.getItem(i);
Element userElem = (Element) elem;
- if (dom.getEventsSunk(userElem) != 0) {
- dom.sinkEvents(userElem, 0);
- }
EventListener listener = dom.getEventListener(userElem);
- // nulls out event listener if and only if it was assigned from our
module
- if (GWT.isScript() && listener != null && isMyListener(listener)) {
+ if (GWT.isScript() && listener != null) {
+ dom.sinkEvents(userElem, 0);
dom.setEventListener(userElem, null);
}
// cleans up DOM-style addEventListener registered handlers
@@ -154,7 +151,9 @@
public abstract int getChildIndex(Element parent, Element child);

public native EventListener getEventListener(Element elem) /*-{
- return elem.__listener;
+ // Return elem.__listener if and only if it was assigned from our
module
+ var maybeListener = elem.__listener;
+ return
@com.google.gwt.user.client.impl.DOMImpl::isMyListener(Ljava/lang/Object;)(maybeListener) ?
maybeListener : null;
}-*/;

public native int getEventsSunk(Element elem) /*-{

--
To view, visit https://gwt-review.googlesource.com/2941
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Matthew Dempsky <mdem...@google.com>

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Fix module unloading with multiple modules on a page
......................................................................

Patch Set 1: Code-Review+1

To make sure it won't break anything else (you're changing the contract of
getEventListener; among other things, it slightly changes what RootPanel
allows in detachOnWindowClose), how about using a private
getRawEventListener with Object as the return type?

That being said, I do think it's a sane change; the above is just a
suggestion.

--
To view, visit https://gwt-review.googlesource.com/2941
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Matthew Dempsky <mdem...@google.com>

Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Ray Cromwell <cromw...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>
Gerrit-HasComments: No

[Matthew Dempsky]

Matthew Dempsky has posted comments on this change.

Change subject: Fix module unloading with multiple modules on a page
......................................................................

Patch Set 1:

(Hm, I thought I already posted this as a comment last night.)

I agree this is technically a semantic change from before, but I think the
risk of it actually breaking anything is low. If getEventListener()
returns an object from another module, there's nothing we can actually do
with it since the methods might have been mangled differently. The only
possibly safe thing to do is to compare against null.

RootPanel does limit itself to only just null testing, but then those
actions are also only done for debugging asserts from what I can tell. So
the risk there seems relatively low.


More generally, Roberto and I discussed that perhaps the compiler could
(optionally, and primarily for development/testing) add appropriate
instanceof checks to JSNI return values. Then we could have caught earlier
on that getEventListener() was returning non-EventListener objects contrary
to its method signature.

--
To view, visit https://gwt-review.googlesource.com/2941
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Matthew Dempsky <mdem...@google.com>

Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

[Ray Cromwell]

Ray Cromwell has posted comments on this change.

Change subject: Fix module unloading with multiple modules on a page
......................................................................

Patch Set 1: Code-Review+2

--
To view, visit https://gwt-review.googlesource.com/2941
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Matthew Dempsky <mdem...@google.com>

[Matthew Dempsky]

Matthew Dempsky has submitted this change and it was merged.

Change subject: Fix module unloading with multiple modules on a page
......................................................................


Fix module unloading with multiple modules on a page

Currently, the GWT compiler in production compiles will optimize away
the instanceof checks in isMyListener() because it assumes
getEventListener() is actually returning an EventListener object.
However, if there are multiple modules loaded on a page, the
__listener property might be for an EventListener from a different
module.

One way to workaround this (the approach taken by this patch) is to
call isMyListener() before getEventListener() returns, so the compiler
can't optimize away the instanceof checks.

Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
---
M user/src/com/google/gwt/user/client/impl/DOMImpl.java
1 file changed, 5 insertions(+), 6 deletions(-)

Approvals:
Ray Cromwell: Looks good to me, approved
Leeroy Jenkins: Verified
Thomas Broyer: Looks good to me, but someone else must approve

Gerrit-MessageType: merged

Gerrit-Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Matthew Dempsky <mdem...@google.com>