Bug 161115

Summary: js/regress/put-by-id-transition-with-indexing-header.html and svg/carto.net/window.svg fail in debug after r204854
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, commit-queue, ggaren, keith_miller, mark.lam, mhahnenb, msaboff, oliver, sam, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
skipping patch
none
the patch keith_miller: review+

Description Filip Pizlo 2016-08-23 16:04:27 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-08-23 16:16:44 PDT
Created attachment 286802 [details]
skipping patch
Comment 2 Filip Pizlo 2016-08-23 16:24:19 PDT
Skipped in http://trac.webkit.org/changeset/204867
Comment 3 Filip Pizlo 2016-08-23 19:27:32 PDT
I'm pretty sure I know what is going on: if we put a new butterfly with more out-of-line capacity (or more pre-capacity) into an object with an old structure (or with m_indexBias reflecting the old pre-capacity) then we won't quite know how to find the base, since that calculation currently relies on the structure and m_indexBias.

This is the code that causes this:

char* JIT_OPERATION operationReallocateButterflyToGrowPropertyStorage(ExecState* exec, JSObject* object, size_t newSize)
{
    VM& vm = exec->vm();
    NativeCallFrameTracer tracer(&vm, exec);

    DeferGC deferGC(vm.heap);
    Butterfly* result = object->growOutOfLineStorage(vm, object->structure()->outOfLineCapacity(), newSize);
    object->setButterflyWithoutChangingStructure(vm, result);
    return reinterpret_cast<char*>(result);
}

Intriguingly, the use of DeferGC is one of the causes. It causes GC to run after we have already set the butterfly, rather than in a state where the object still points to the "right" butterfly for its structure.

I'm tempted to say that the solution is to simply remove the DeferGC!  If we do that then the GC will happen exactly where we want it to: inside growOutOfLineStorage().  That's a fine place to GC, since the object will still be in a sane state in that method.
Comment 4 Filip Pizlo 2016-08-23 21:32:04 PDT
*** Bug 161114 has been marked as a duplicate of this bug. ***
Comment 5 Filip Pizlo 2016-08-23 21:32:41 PDT
I have fixes for these crashes, I'm testing them now.
Comment 6 Filip Pizlo 2016-08-23 23:21:39 PDT
Created attachment 286835 [details]
the patch
Comment 7 Keith Miller 2016-08-24 09:55:43 PDT
Comment on attachment 286835 [details]
the patch

r=me.
Comment 8 Filip Pizlo 2016-08-24 09:57:50 PDT
Landed in https://trac.webkit.org/changeset/204901