Bug 22504 - Crashes during code generation occur due to refing of ignoredResult()
Summary: Crashes during code generation occur due to refing of ignoredResult()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-25 17:55 PST by Cameron Zwarich (cpst)
Modified: 2008-12-02 22:14 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (without tests) (14.73 KB, patch)
2008-11-27 02:45 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (18.33 KB, patch)
2008-11-28 23:49 PST, Cameron Zwarich (cpst)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-11-25 17:55:02 PST
One thing seems to have been slightly overlooked in the handling of ignoredResult(). Many functions pass their value of dst to BytecodeGenerator::emitNode() and then put its return value into a RefPtr. Unfortunately, many emitBytecode() Node member functions return their passed value of dst, which leads to a crash when attempting to increase the reference count of ignoredResult(). Here is an example of this:

function f()
{
    do
        try { } catch (o) { }
    while (false) { }
}

It seems that this problem was known, because there are many emitBytecode() functions that do something like

    if (dst == ignoredResult())
        dst = 0;

However, this seems like a suboptimal solution to the problem.
Comment 1 Cameron Zwarich (cpst) 2008-11-25 19:25:22 PST
Geoff told me a pretty good solution to this bug. Make ignoredResult() an instance-specific register off of BytecodeGenerator, so that reffing it is legal. I think I will implement that.
Comment 2 Cameron Zwarich (cpst) 2008-11-27 02:45:53 PST
Created attachment 25556 [details]
Proposed patch (without tests)

I'm gonna make some tests now.
Comment 3 Cameron Zwarich (cpst) 2008-11-28 23:49:41 PST
Created attachment 25586 [details]
Proposed patch

This includes all of the cases that will crash that I am aware of.
Comment 4 Eric Seidel (no email) 2008-12-01 12:31:06 PST
Comment on attachment 25586 [details]
Proposed patch

This looks sane to me.  But one of the SFX folks should probably review it.
Comment 5 Geoffrey Garen 2008-12-01 17:55:33 PST
Comment on attachment 25586 [details]
Proposed patch

r=me
Comment 6 Cameron Zwarich (cpst) 2008-12-02 22:14:43 PST
Landed in r38930.