[gwt-contrib] Change in gwt[master]: Fix module unloading with multiple modules on a page
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
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: merged
Gerrit-Change-Id: I25d9471c1e14756196a11223eadb765ed303a3b4
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Matthew Dempsky mdemp...@google.com
Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org
Gerrit-Reviewer: Matthew Dempsky mdemp...@google.com
Gerrit-Reviewer: Ray Cromwell cromwell...@google.com
Gerrit-Reviewer: Thomas Broyer t.bro...@gmail.com
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups GWT Contributors group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Fix module unloading with multiple modules on a page
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 mdemp...@google.com Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org Gerrit-Reviewer: Matthew Dempsky mdemp...@google.com Gerrit-Reviewer: Ray Cromwell cromwell...@google.com Gerrit-Reviewer: Thomas Broyer t.bro...@gmail.com Gerrit-HasComments: No -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Fix module unloading with multiple modules on a page
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 mdemp...@google.com Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org Gerrit-Reviewer: Matthew Dempsky mdemp...@google.com Gerrit-Reviewer: Ray Cromwell cromwell...@google.com Gerrit-Reviewer: Thomas Broyer t.bro...@gmail.com Gerrit-HasComments: No -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Fix module unloading with multiple modules on a page
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 mdemp...@google.com
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups GWT Contributors group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Fix module unloading with multiple modules on a page
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 mdemp...@google.com Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org Gerrit-Reviewer: Ray Cromwell cromwell...@google.com Gerrit-Reviewer: Thomas Broyer t.bro...@gmail.com Gerrit-HasComments: No -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
