Title: [115594] trunk/Source/WebCore
- Revision
- 115594
- Author
- [email protected]
- Date
- 2012-04-29 01:07:50 -0700 (Sun, 29 Apr 2012)
Log Message
REGRESSION(r113086): onresize event handler can be deleted in popup window
https://bugs.webkit.org/show_bug.cgi?id=84908
Reviewed by Ojan Vafai.
In a nutshell, an onresize event handler in the popup window
can be non-deterministically deleted. For more details, please
look at Chromium issue 123642:
http://code.google.com/p/chromium/issues/detail?id=123642
I confirmed that this bug is the regression caused by r113086.
r113086 introduced the following code:
void V8LazyEventListener::prepareListenerObject(...) {
if (hasExistingListenerObject())
return;
...;
// Since we only parse once, there's no need to keep data
// used for parsing around anymore.
m_functionName = String();
m_code = String();
m_eventParameterName = String();
m_sourceURL = String();
setListenerObject(wrappedFunction);
}
This is not correct. The parsing can be done more than once,
and thus we cannot clear data. This patch removes the above code.
Consider the following situation:
(1) Assume '<body _onresize_="f()"></body>'.
(2) prepareListenerObject() runs.
(3) Since this is the first parsing, hasExistingListenerObject()
returns false. After the parsing, the listener object is set
by setListenerObject().
(4) GC runs. Since there is no strong reference to the listener
object, weakEventListenerCallback() is called back, and the listener
object is disposed.
(5) A resize event is triggered.
(6) prepareListenerObject() is called again. Since the listener object
is already disposed, hasExistingListenerObject() returns false,
and the second parsing starts.
In my investigation, the above situation is happening in the reported
Chromium bug. Anyway, I am sure that potentially the parsing can be
done more than once, and thus we must keep m_xxxx data.
However, this is just a temporary fix. We should fix the code so that
an alive event listener object is never reclaimed.
See https://bugs.webkit.org/show_bug.cgi?id=85152 for more details.
No tests: I tried hard to create a DRT test, but could not.
The bug depends on the behavior of GC, and thus the reported bug is
non-deterministic. For example, (as explained in the Chromium issue,)
the bug does not happen if we load an HTML from network because
the network latency hides the bug. Also the bug happens in the
popup window only. If we open the reported HTML in the main window,
we cannot reproduce the bug.
* bindings/v8/V8LazyEventListener.cpp:
(WebCore::V8LazyEventListener::prepareListenerObject):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (115593 => 115594)
--- trunk/Source/WebCore/ChangeLog 2012-04-29 07:09:36 UTC (rev 115593)
+++ trunk/Source/WebCore/ChangeLog 2012-04-29 08:07:50 UTC (rev 115594)
@@ -1,3 +1,70 @@
+2012-04-29 Kentaro Hara <[email protected]>
+
+ REGRESSION(r113086): onresize event handler can be deleted in popup window
+ https://bugs.webkit.org/show_bug.cgi?id=84908
+
+ Reviewed by Ojan Vafai.
+
+ In a nutshell, an onresize event handler in the popup window
+ can be non-deterministically deleted. For more details, please
+ look at Chromium issue 123642:
+ http://code.google.com/p/chromium/issues/detail?id=123642
+
+ I confirmed that this bug is the regression caused by r113086.
+
+ r113086 introduced the following code:
+
+ void V8LazyEventListener::prepareListenerObject(...) {
+ if (hasExistingListenerObject())
+ return;
+ ...;
+ // Since we only parse once, there's no need to keep data
+ // used for parsing around anymore.
+ m_functionName = String();
+ m_code = String();
+ m_eventParameterName = String();
+ m_sourceURL = String();
+
+ setListenerObject(wrappedFunction);
+ }
+
+ This is not correct. The parsing can be done more than once,
+ and thus we cannot clear data. This patch removes the above code.
+
+ Consider the following situation:
+
+ (1) Assume '<body _onresize_="f()"></body>'.
+ (2) prepareListenerObject() runs.
+ (3) Since this is the first parsing, hasExistingListenerObject()
+ returns false. After the parsing, the listener object is set
+ by setListenerObject().
+ (4) GC runs. Since there is no strong reference to the listener
+ object, weakEventListenerCallback() is called back, and the listener
+ object is disposed.
+ (5) A resize event is triggered.
+ (6) prepareListenerObject() is called again. Since the listener object
+ is already disposed, hasExistingListenerObject() returns false,
+ and the second parsing starts.
+
+ In my investigation, the above situation is happening in the reported
+ Chromium bug. Anyway, I am sure that potentially the parsing can be
+ done more than once, and thus we must keep m_xxxx data.
+
+ However, this is just a temporary fix. We should fix the code so that
+ an alive event listener object is never reclaimed.
+ See https://bugs.webkit.org/show_bug.cgi?id=85152 for more details.
+
+ No tests: I tried hard to create a DRT test, but could not.
+ The bug depends on the behavior of GC, and thus the reported bug is
+ non-deterministic. For example, (as explained in the Chromium issue,)
+ the bug does not happen if we load an HTML from network because
+ the network latency hides the bug. Also the bug happens in the
+ popup window only. If we open the reported HTML in the main window,
+ we cannot reproduce the bug.
+
+ * bindings/v8/V8LazyEventListener.cpp:
+ (WebCore::V8LazyEventListener::prepareListenerObject):
+
2012-04-28 Sam Weinig <[email protected]>
Smooth scrolling needs a new key
Modified: trunk/Source/WebCore/bindings/v8/V8LazyEventListener.cpp (115593 => 115594)
--- trunk/Source/WebCore/bindings/v8/V8LazyEventListener.cpp 2012-04-29 07:09:36 UTC (rev 115593)
+++ trunk/Source/WebCore/bindings/v8/V8LazyEventListener.cpp 2012-04-29 08:07:50 UTC (rev 115594)
@@ -59,6 +59,9 @@
, m_sourceURL(sourceURL)
, m_node(node)
, m_position(position)
+#ifndef NDEBUG
+ , m_prepared(false)
+#endif
{
}
@@ -216,11 +219,20 @@
wrappedFunction->SetName(v8::String::New(fromWebCoreString(m_functionName), m_functionName.length()));
- // Since we only parse once, there's no need to keep data used for parsing around anymore.
- m_functionName = String();
- m_code = String();
- m_eventParameterName = String();
- m_sourceURL = String();
+ // FIXME: Remove m_prepared and the following comment-outs.
+ // See https://bugs.webkit.org/show_bug.cgi?id=85152 for more details.
+#ifndef NDEBUG
+ // Checks if the second parsing never happens. Currently the second parsing can happen
+ // in a popup window.
+ ASSERT(!m_prepared);
+ m_prepared = true;
+#endif
+ // Comments out the following code since the second parsing can happen.
+ // // Since we only parse once, there's no need to keep data used for parsing around anymore.
+ // m_functionName = String();
+ // m_code = String();
+ // m_eventParameterName = String();
+ // m_sourceURL = String();
setListenerObject(wrappedFunction);
}
Modified: trunk/Source/WebCore/bindings/v8/V8LazyEventListener.h (115593 => 115594)
--- trunk/Source/WebCore/bindings/v8/V8LazyEventListener.h 2012-04-29 07:09:36 UTC (rev 115593)
+++ trunk/Source/WebCore/bindings/v8/V8LazyEventListener.h 2012-04-29 08:07:50 UTC (rev 115594)
@@ -75,6 +75,9 @@
String m_sourceURL;
Node* m_node;
TextPosition m_position;
+#ifndef NDEBUG
+ bool m_prepared;
+#endif
};
} // namespace WebCore
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
