Bug 84908

Summary:

REGRESSION(r113086): onresize event handler can be deleted in popup window

Product:

WebKit

Reporter:

Kentaro Hara <haraken>

Component:

WebCore JavaScript

Assignee:

Kentaro Hara <haraken>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

abarth, ap, arv, ggaren, japhet, levin, ojan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

85208

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Rebased patch	none
Patch	none
patch for landing	none

Kentaro Hara

Reported 2012-04-25 16:55:42 PDT

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.

Attachments
Patch (3.91 KB, patch) 2012-04-25 16:56 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Rebased patch (5.15 KB, patch) 2012-04-26 17:19 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Patch (3.93 KB, patch) 2012-04-27 10:10 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
patch for landing (5.51 KB, patch) 2012-04-28 23:21 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Kentaro Hara

Comment 1 2012-04-25 16:56:31 PDT

Created attachment 138900 [details] Patch

Kentaro Hara

Comment 2 2012-04-25 16:57:35 PDT

The Chromium bug is marked as ReleaseBlock-Stable. I am happy if I could get a review quickly:)

Erik Arvidsson

Comment 3 2012-04-25 17:07:13 PDT

Comment on attachment 138900 [details] Patch How does this work in JSC? Can we add a test please?

Kentaro Hara

Comment 4 2012-04-25 17:12:50 PDT

(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?

Erik Arvidsson

Comment 5 2012-04-25 18:50:26 PDT

(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

Kentaro Hara

Comment 6 2012-04-26 10:41:56 PDT

(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...

Kentaro Hara

Comment 7 2012-04-26 10:51:02 PDT

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?

Erik Arvidsson

Comment 8 2012-04-26 10:56:02 PDT

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.

Kentaro Hara

Comment 9 2012-04-26 10:59:31 PDT

(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.

Erik Arvidsson

Comment 10 2012-04-26 10:59:54 PDT

(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!

Kentaro Hara

Comment 11 2012-04-26 16:49:40 PDT

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.

Erik Arvidsson

Comment 12 2012-04-26 17:12:41 PDT

Popups are testable but I suspect chromium DRT is doing something different because when I do exactly the same thing in DRT it works.

Kentaro Hara

Comment 13 2012-04-26 17:19:55 PDT

Created attachment 139102 [details] Rebased patch

Erik Arvidsson

Comment 14 2012-04-27 09:47:02 PDT

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?

Kentaro Hara

Comment 15 2012-04-27 09:53:29 PDT

(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.

Kentaro Hara

Comment 16 2012-04-27 10:10:20 PDT

Created attachment 139222 [details] Patch

Ojan Vafai

Comment 17 2012-04-27 19:39:57 PDT

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)?

Kentaro Hara

Comment 18 2012-04-28 03:41:39 PDT

(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...)

Ojan Vafai

Comment 19 2012-04-28 17:34:19 PDT

(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?

Kentaro Hara

Comment 20 2012-04-28 23:21:24 PDT

Created attachment 139392 [details] patch for landing

WebKit Review Bot

Comment 21 2012-04-29 01:07:08 PDT

Comment on attachment 139392 [details] patch for landing Clearing flags on attachment: 139392 Committed r115594: <http://trac.webkit.org/changeset/115594>

Note You need to log in before you can comment on or make changes to this bug.