[webkit-reviews] review requested: [Bug 16868] Gmail crash : [Attachment 18452] Proposed patch

[bugzilla-daemon at webkit.org]

Cameron Zwarich (cpst) <cwzwarich at uwaterloo.ca> has asked  for review:
Bug 16868: Gmail crash
http://bugs.webkit.org/show_bug.cgi?id=16868

Attachment 18452: Proposed patch
http://bugs.webkit.org/attachment.cgi?id=18452&action=edit

------- Additional Comments from Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>
Maciej and I debugged this crash for a while, and we came up with two problems:


1) There is no tear-off in the case of cross-window eval().

2) If m_savedExec is different than m_callingExec, then this can cause some
ActivationImp objects to be missed by the garbage collector. This may have been
a very obscure issue before the tear-off patch, but the tear-off patch takes
the majority of ActivationImp objects out of the JS heap, so the conservative
collector won't catch any mistakes.

This patch fixes both of these issues. It has been tested with both debug and
release settings and passes all tests. I should be able to construct a layout
test soon for the first issue, but probably not for the second.

One strange thing is that you can't fix the bug by only calling mark() on
m_savedExec if it is different than m_callingExec. You also need to call it
when it is the same, presumedly to reach a distinct m_savedExec further back
down the stack. This might be a potential performance issue, especially in the
way it is written here. It is probably a good idea to rewrite this patch so it
goes down the callingExec and savedExec chains in parallel, and only recurses
when they become distinct, but I have to go to bed. Maciej suggested modifying
WebCore so that we can remove m_savedExec entirely. However, that might be a
bit of work.

We should also investigate the relationship between this bug and bug 16871.