In a nutshell, an onresize event handler in the popup window can be non-deterministically deleted. For more details, please look at the chromium issue 123642: http://code.google.com/p/chromium/issues/detail?id=123642 I confirmed that this is the regression caused by r113086.
Created attachment 138900 [details] Patch
The Chromium bug is marked as ReleaseBlock-Stable. I am happy if I could get a review quickly:)
Comment on attachment 138900 [details] Patch How does this work in JSC? Can we add a test please?
(In reply to comment #3) > (From update of attachment 138900 [details]) > How does this work in JSC? JSC works fine. r113086 has changed the V8 bindings only. > Can we add a test please? As I explained in the ChangeLog, I couldn't make the DRT test case. For example, I wrote a test case like this, but could not reproduce it: <body onclick="f()"> <script> function gc() { ... } function f() { document.body.innerHTML += "onclick!<br>"; gc(); } </script> </body> I tried other test cases, but it seems that we cannot reproduce the bug in the main window. It just happens in the popup window, as far as I observed. Also the latency to load the HTML seems to be important. The bug happens only when we load the HTML from the disk. Do you have any idea?
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 138900 [details] [details]) > > How does this work in JSC? > > JSC works fine. r113086 has changed the V8 bindings only. Yes, but JSC also clears out these strings. Do they keep their listener object alive where we do not? > > Can we add a test please? > > As I explained in the ChangeLog, I couldn't make the DRT test case. For example, I wrote a test case like this, but could not reproduce it: > > <body onclick="f()"> > <script> > function gc() { ... } > function f() { document.body.innerHTML += "onclick!<br>"; gc(); } > </script> > </body> > > I tried other test cases, but it seems that we cannot reproduce the bug in the main window. It just happens in the popup window, as far as I observed. Also the latency to load the HTML seems to be important. The bug happens only when we load the HTML from the disk. Do you have any idea? I'll try a few things
(In reply to comment #5) > > JSC works fine. r113086 has changed the V8 bindings only. > > Yes, but JSC also clears out these strings. Do they keep their listener object alive where we do not? I don't know but I think > Do they keep their listener object alive where we do not? ^^^ this is possible. The timing of when JSC/V8 calls back weakCallback() seems completely different between JSC and V8. Though I am not understanding JSC's GC much enough to explain the details... Anyway the bug depends on the timing of when the listener object gets weaken by GC, which _can_ be different between JSC and V8. I hope we could make some test cases to convince it...
I agree that (1) we should fix V8's GC about when weakEventListenerCallback() is called and that (2) we want a test case. But personally I think that landing the uploaded patch would be acceptable: - This bug is causing serious problem in full-screen apps in Chromium. The Chromium issue is marked as P1 and Release-block-stable. - The patch just changes the listener object to keep data until the listener object is reclaimed. This won't cause any memory leak. ojan, levin, arv: any idea?
I was hammering on this last night for an hour or so and I could not reproduce the behavior. If we have a repro it should be easier to figure out why this happens.
(In reply to comment #8) > I was hammering on this last night for an hour or so and I could not reproduce the behavior. I can reproduce it by following the steps in http://code.google.com/p/chromium/issues/detail?id=123642 > If we have a repro it should be easier to figure out why this happens. I described what is happening in the ChangeLog. The problem is that we do not know why V8's GC calls back weakEventListenerObject(), even if onresize="f()" is still registered to the <body> tag.
(In reply to comment #8) > I was hammering on this last night for an hour or so and I could not reproduce the behavior. If we have a repro it should be easier to figure out why this happens. I'm stupid. I should have tried the attachments. innerHTML += strikes again!
As far as I've tried, I could not reproduce the bug in the main window... It does happen in popup window only, which is not testable.
Popups are testable but I suspect chromium DRT is doing something different because when I do exactly the same thing in DRT it works.
Created attachment 139102 [details] Rebased patch
Comment on attachment 139102 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=139102&action=review This solution does not seem right. Why are we calling listener->disposeListenerObject() when the listener object should be kept alive? > Source/WebCore/ChangeLog:-29520 > -2012-04-03 Erik Arvidsson <arv@chromium.org> Why are you removing this?
(In reply to comment #14) > This solution does not seem right. Why are we calling listener->disposeListenerObject() when the listener object should be kept alive? Rather, the problem would be the fact that weakEventListenerCallback() is called by V8's GC. (It is reasonable to call listener->disposeListenerObject() given that weakEventListenerCallback() is call backed.) Maybe we are forgetting to add some hidden reference (or something like that) to the listener object? I think - the right fix would be to make V8's GC not call back weakEventListenerCallback() when the listener object should be kept alive. - if it will take a long time to make the right fix, it would be acceptable to land the patch for now since the patch will be harmless. > > Source/WebCore/ChangeLog:-29520 > > -2012-04-03 Erik Arvidsson <arv@chromium.org> > > Why are you removing this? Sorry, will fix.
Created attachment 139222 [details] Patch
Comment on attachment 139222 [details] Patch This is fine for now to fix the regression. We really should fix this if we can though. In the meantime, can you add an assert and a FIXME (as well as file a bug)?
(In reply to comment #17) > (From update of attachment 139222 [details]) > This is fine for now to fix the regression. We really should fix this if we can though. In the meantime, can you add an assert and a FIXME (as well as file a bug)? ojan: Thanks! I'll file a bug and add a FIXME. But do you have any idea to insert ASSERT? (We might not want to introduce a new member variable just to count how many times prepareListenerObject() is called...)
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 139222 [details] [details]) > > This is fine for now to fix the regression. We really should fix this if we can though. In the meantime, can you add an assert and a FIXME (as well as file a bug)? > > ojan: Thanks! I'll file a bug and add a FIXME. But do you have any idea to insert ASSERT? (We might not want to introduce a new member variable just to count how many times prepareListenerObject() is called...) We could add it just for debug?
Created attachment 139392 [details] patch for landing
Comment on attachment 139392 [details] patch for landing Clearing flags on attachment: 139392 Committed r115594: <http://trac.webkit.org/changeset/115594>