Bug 84908 - REGRESSION(r113086): onresize event handler can be deleted in popup window
Summary: REGRESSION(r113086): onresize event handler can be deleted in popup window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Kentaro Hara
URL:
Keywords:
Depends on: 85208
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-25 16:55 PDT by Kentaro Hara
Modified: 2012-04-30 10:46 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 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.
Comment 1 Kentaro Hara 2012-04-25 16:56:31 PDT
Created attachment 138900 [details]
Patch
Comment 2 Kentaro Hara 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:)
Comment 3 Erik Arvidsson 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?
Comment 4 Kentaro Hara 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?
Comment 5 Erik Arvidsson 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
Comment 6 Kentaro Hara 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...
Comment 7 Kentaro Hara 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?
Comment 8 Erik Arvidsson 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.
Comment 9 Kentaro Hara 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.
Comment 10 Erik Arvidsson 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!
Comment 11 Kentaro Hara 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.
Comment 12 Erik Arvidsson 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.
Comment 13 Kentaro Hara 2012-04-26 17:19:55 PDT
Created attachment 139102 [details]
Rebased patch
Comment 14 Erik Arvidsson 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?
Comment 15 Kentaro Hara 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.
Comment 16 Kentaro Hara 2012-04-27 10:10:20 PDT
Created attachment 139222 [details]
Patch
Comment 17 Ojan Vafai 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)?
Comment 18 Kentaro Hara 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...)
Comment 19 Ojan Vafai 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?
Comment 20 Kentaro Hara 2012-04-28 23:21:24 PDT
Created attachment 139392 [details]
patch for landing
Comment 21 WebKit Review Bot 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>