Bug 149965 – REGRESSION: ASSERT (impl->isAtomic()) @ facebook.com

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Geoffrey Garen]

STEPS TO REPRODUCE:
1. Create a debug build
2. Visit Facebook.com
3. Wait about 5 seconds

ASSERTION FAILED: impl->isAtomic()
/Volumes/Big/ggaren/OpenSource/Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp(261) : bool JSC::DFG::ConstantFoldingPhase::foldConstants(JSC::DFG::BasicBlock *)
1   0x114645330 WTFCrash
2   0x113d2b395 JSC::DFG::ConstantFoldingPhase::foldConstants(JSC::DFG::BasicBlock*)
3   0x113d2a39a JSC::DFG::ConstantFoldingPhase::run()
4   0x113d2a215 bool JSC::DFG::runAndLog<JSC::DFG::ConstantFoldingPhase>(JSC::DFG::ConstantFoldingPhase&)
5   0x113d2a19e bool JSC::DFG::runPhase<JSC::DFG::ConstantFoldingPhase>(JSC::DFG::Graph&)
6   0x113d2a158 JSC::DFG::performConstantFolding(JSC::DFG::Graph&)
7   0x113ea1bd1 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
8   0x113ea0cda JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
9   0x113fa60d0 JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*)
10  0x113fa45c4 JSC::DFG::Worklist::threadFunction(void*)
11  0x1146b0419 WTF::createThread(void (*)(void*), void*, char const*)::$_0::operator()() const
12  0x1146b03ec std::__1::__function::__func<WTF::createThread(void (*)(void*), void*, char const*)::$_0, std::__1::allocator<WTF::createThread(void (*)(void*), void*, char const*)::$_0>, void ()>::operator()()
13  0x1140d91da std::__1::function<void ()>::operator()() const
14  0x1146af36e WTF::threadEntryPoint(void*)
15  0x1146b0cf8 WTF::wtfThreadEntryPoint(void*)
16  0x7fff8e3ba268 _pthread_body
17  0x7fff8e3ba1e5 _pthread_body
18  0x7fff8e3b841d thread_start

[Geoffrey Garen]

I debugged this a little.

In the crashing case, the AbstractInterpreter executes CheckIdent and *does not* find a constant. Then, the DFGConstantFoldingPhase executes CheckIdent and *does* find a constant. The DFGConstantFoldingPhase constant is string equal to the uid expected by CheckIdent, but it is a plain string and not an atomic string.

[Filip Pizlo]

(In reply to comment #1)
> I debugged this a little.
> 
> In the crashing case, the AbstractInterpreter executes CheckIdent and *does
> not* find a constant. Then, the DFGConstantFoldingPhase executes CheckIdent
> and *does* find a constant. The DFGConstantFoldingPhase constant is string
> equal to the uid expected by CheckIdent, but it is a plain string and not an
> atomic string.

It's more subtle than that.

The constant folding phase code is assuming that it is running after m_interpreter.execute().  It's saying things like, "oh hey this has to be an atomic string because we already did StringIdentUse filtering".  It's true that if you had run edge filtering first, then StringIdentUse would have filtered an abstract value containing a plain string to the empty abstract value (representing the fact that we would have always exited).  And that's exactly what happens inside AbstractInterpreter - the executeEffects() method that has the CheckIdent case runs after edge filtering, so the AbstractInterpreter sees an empty AbstractValue because the StringIdentUse filtered the non-ident string to the empty value.

The solution is to fix the constant folder to no longer assume that edge filtering already ran.  If it sees a constant value, it should verify that the constant value is valid (i.e. would have passed edge filtering, i.e. would have been an atomic string).

Probably just replacing:

ASSERT(impl->isAtomic());

with:

if (!impl->isAtomic())
    break;

would go a long way.

[Yusuke Suzuki]

(In reply to comment #2)
> (In reply to comment #1)
> > I debugged this a little.
> > 
> > In the crashing case, the AbstractInterpreter executes CheckIdent and *does
> > not* find a constant. Then, the DFGConstantFoldingPhase executes CheckIdent
> > and *does* find a constant. The DFGConstantFoldingPhase constant is string
> > equal to the uid expected by CheckIdent, but it is a plain string and not an
> > atomic string.
> 
> It's more subtle than that.
> 
> The constant folding phase code is assuming that it is running after
> m_interpreter.execute().  It's saying things like, "oh hey this has to be an
> atomic string because we already did StringIdentUse filtering".  It's true
> that if you had run edge filtering first, then StringIdentUse would have
> filtered an abstract value containing a plain string to the empty abstract
> value (representing the fact that we would have always exited).  And that's
> exactly what happens inside AbstractInterpreter - the executeEffects()
> method that has the CheckIdent case runs after edge filtering, so the
> AbstractInterpreter sees an empty AbstractValue because the StringIdentUse
> filtered the non-ident string to the empty value.
> 
> The solution is to fix the constant folder to no longer assume that edge
> filtering already ran.  If it sees a constant value, it should verify that
> the constant value is valid (i.e. would have passed edge filtering, i.e.
> would have been an atomic string).
> 
> Probably just replacing:
> 
> ASSERT(impl->isAtomic());
> 
> with:
> 
> if (!impl->isAtomic())
>     break;
> 
> would go a long way.

[Yusuke Suzuki]

Thanks. I'm now investigating to create the reproducible test cases (still did not succeed yet).

[Filip Pizlo]

(In reply to comment #4)
> Thanks. I'm now investigating to create the reproducible test cases (still
> did not succeed yet).

This is the kind of bug where it's extremely valuable to land a fix, even if it does not have a test case.

[Yusuke Suzuki]

(In reply to comment #5)
> (In reply to comment #4)
> > Thanks. I'm now investigating to create the reproducible test cases (still
> > did not succeed yet).
> 
> This is the kind of bug where it's extremely valuable to land a fix, even if
> it does not have a test case.

Thanks. OK, I'll upload the patch :)

[Yusuke Suzuki]

Created attachment 262900 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 262900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262900&action=review

Added comment :)

> Source/JavaScriptCore/ChangeLog:15
> +        in AI to the same in the constant folding phase.

If my understanding is not correct, please point it :)

In AI, we execute `executeEdges`. Finally, this executes AbstractValue::filter(with Use filter). And it makes the value empty if the value does not meet to the edge filter.
So essentially, in AI, this change is not required. AI makes the constant value JSEmpty when it does not meet to the filter.
I changed the code to keep it in sync with the code in the constant folding phase.
In constant folding phase, we extract the constant value before executing AI's execute. So it means we may see the non-edge-filtered value.

[Geoffrey Garen]

Comment on attachment 262900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262900&action=review

>> Source/JavaScriptCore/ChangeLog:15
>> +        in AI to the same in the constant folding phase.
> 
> If my understanding is not correct, please point it :)
> 
> In AI, we execute `executeEdges`. Finally, this executes AbstractValue::filter(with Use filter). And it makes the value empty if the value does not meet to the edge filter.
> So essentially, in AI, this change is not required. AI makes the constant value JSEmpty when it does not meet to the filter.
> I changed the code to keep it in sync with the code in the constant folding phase.
> In constant folding phase, we extract the constant value before executing AI's execute. So it means we may see the non-edge-filtered value.

I think your explanation in this ChangeLog is correct -- and indeed, if I only apply the constant folding change, and not the AI change, facebook.com stops crashing.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2307
>      case CheckIdent: {
>          AbstractValue& value = forNode(node->child1());
>          UniquedStringImpl* uid = node->uidOperand();
> -        ASSERT(uid->isSymbol() ? !(value.m_type & ~SpecSymbol) : !(value.m_type & ~SpecStringIdent)); // Edge filtering should have already ensured this.
> +        const UniquedStringImpl* constantUid = nullptr;
>  
>          JSValue childConstant = value.value();
>          if (childConstant) {
>              if (uid->isSymbol()) {
> -                ASSERT(childConstant.isSymbol());
> -                if (asSymbol(childConstant)->privateName().uid() == uid) {
> -                    m_state.setFoundConstants(true);
> -                    break;
> -                }
> +                if (childConstant.isSymbol())
> +                    constantUid = asSymbol(childConstant)->privateName().uid();
>              } else {
> -                ASSERT(childConstant.isString());
> -                if (asString(childConstant)->tryGetValueImpl() == uid) {
> -                    m_state.setFoundConstants(true);
> -                    break;
> +                if (childConstant.isString()) {
> +                    if (const auto* impl = asString(childConstant)->tryGetValueImpl()) {
> +                        // Edge filtering requires that a value here should be StringIdent.
> +                        // However, a constant value propagated in DFG is not filtered.
> +                        // So here, we check the propagated value is actually an atomic string.
> +                        // And if it's not, we just ignore.
> +                        if (impl->isAtomic())
> +                            constantUid = static_cast<const UniquedStringImpl*>(impl);
> +                    }
>                  }
>              }
>          }
>  
> +        if (uid == constantUid) {
> +            m_state.setFoundConstants(true);
> +            break;
> +        }

I'm not sure about this change. Consistency is nice. But it's a bit confusing to have handling here for conditions that can never arise.

I think this patch might be better if you just make the change to the constant folding phase.

I wonder what Phil thinks.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:265
> +                                // And if it's not, we just ignore.
> +                                if (impl->isAtomic())
> +                                    constantUid = static_cast<const UniquedStringImpl*>(impl);

Is it possible to test this? I think all we need is an inferred constant that is not an identifier in program text. Something like this:

function get(object, descriptor)
{
    return object[descriptor.propertyName];
}

noInline(get);

var propertyName = String("propertyName" + (new Date).getSeconds());

function Descriptor () { }
Descriptor.prototype.propertyName = propertyName;

var object = { propertyName: 0 };
var descriptor = new Descriptor;

for (var i = 0; i < 100000; ++i)
    get(object, descriptor);

[Filip Pizlo]

> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2307
> >      case CheckIdent: {
> >          AbstractValue& value = forNode(node->child1());
> >          UniquedStringImpl* uid = node->uidOperand();
> > -        ASSERT(uid->isSymbol() ? !(value.m_type & ~SpecSymbol) : !(value.m_type & ~SpecStringIdent)); // Edge filtering should have already ensured this.
> > +        const UniquedStringImpl* constantUid = nullptr;
> >  
> >          JSValue childConstant = value.value();
> >          if (childConstant) {
> >              if (uid->isSymbol()) {
> > -                ASSERT(childConstant.isSymbol());
> > -                if (asSymbol(childConstant)->privateName().uid() == uid) {
> > -                    m_state.setFoundConstants(true);
> > -                    break;
> > -                }
> > +                if (childConstant.isSymbol())
> > +                    constantUid = asSymbol(childConstant)->privateName().uid();
> >              } else {
> > -                ASSERT(childConstant.isString());
> > -                if (asString(childConstant)->tryGetValueImpl() == uid) {
> > -                    m_state.setFoundConstants(true);
> > -                    break;
> > +                if (childConstant.isString()) {
> > +                    if (const auto* impl = asString(childConstant)->tryGetValueImpl()) {
> > +                        // Edge filtering requires that a value here should be StringIdent.
> > +                        // However, a constant value propagated in DFG is not filtered.
> > +                        // So here, we check the propagated value is actually an atomic string.
> > +                        // And if it's not, we just ignore.
> > +                        if (impl->isAtomic())
> > +                            constantUid = static_cast<const UniquedStringImpl*>(impl);
> > +                    }
> >                  }
> >              }
> >          }
> >  
> > +        if (uid == constantUid) {
> > +            m_state.setFoundConstants(true);
> > +            break;
> > +        }
> 
> I'm not sure about this change. Consistency is nice. But it's a bit
> confusing to have handling here for conditions that can never arise.
> 
> I think this patch might be better if you just make the change to the
> constant folding phase.
> 
> I wonder what Phil thinks.

I agree.

In AbstractInterpreter::executeEffects(), we know that we have already executed executeEdges(), and so it is correct (and preferable) to assert that the state is as it should be after edges are executed.

In ConstantFoldingPhase, for this particular node, we are not calling m_interpreter.execute().  Therefore, we are replacing all of the logic in both executeEdges() and executeEffects().  So, we cannot assume that executeEdges() has run.

It makes no sense to have consistency between code that is replacing execute() and code that loves in executeEffects().  If you're replacing execute(), then you are guaranteed that executeEdges() has not been called.  If you're implementing executeEffect(), then you are guaranteed that executeEdges() has been called.  The code must not look the same!

[Geoffrey Garen]

Comment on attachment 262900 [details]
Patch

OK, please rework this patch to remove the changes to the constant folding phase, and to add a test if possible.

[Filip Pizlo]

(In reply to comment #11)
> Comment on attachment 262900 [details]
> Patch
> 
> OK, please rework this patch to remove the changes to the constant folding
> phase, and to add a test if possible.

*remove the changes to the abstract interpreter. The changes to constant folding are the necessary ones.

[Yusuke Suzuki]

(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 262900 [details]
> > Patch
> > 
> > OK, please rework this patch to remove the changes to the constant folding
> > phase, and to add a test if possible.
> 
> *remove the changes to the abstract interpreter. The changes to constant
> folding are the necessary ones.


Thanks. OK, I'll drop AI part in the above patch :)

[Yusuke Suzuki]

Comment on attachment 262900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262900&action=review

>> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:265
>> +                                    constantUid = static_cast<const UniquedStringImpl*>(impl);
> 
> Is it possible to test this? I think all we need is an inferred constant that is not an identifier in program text. Something like this:
> 
> function get(object, descriptor)
> {
>     return object[descriptor.propertyName];
> }
> 
> noInline(get);
> 
> var propertyName = String("propertyName" + (new Date).getSeconds());
> 
> function Descriptor () { }
> Descriptor.prototype.propertyName = propertyName;
> 
> var object = { propertyName: 0 };
> var descriptor = new Descriptor;
> 
> for (var i = 0; i < 100000; ++i)
>     get(object, descriptor);

In the above case, we extract the constant value from descriptor.propertyName. So it becomes JSConstant (And it becomes atomic string because when using it in object[expr], it attempt to atomize the given string).
Hm, in the meantime, I'll just upload the revised patch...

[Yusuke Suzuki]

Created attachment 262987 [details]
Patch

[Geoffrey Garen]

Comment on attachment 262987 [details]
Patch

r=me

By the way, considering optimization:

(1) I wonder how Facebook produces this constant string that is somehow not an identifier. Usually, we try to make constant strings identifiers.

(2) I believe we *could* optimize this case to notice that the two strings, though not pointer equal, are character equal, and do the constant folding anyway.

Probably not a big deal, but I thought I would share the idea.

[Yusuke Suzuki]

(In reply to comment #16)
> Comment on attachment 262987 [details]
> Patch
> 
> r=me
> 
> By the way, considering optimization:
> 
> (1) I wonder how Facebook produces this constant string that is somehow not
> an identifier. Usually, we try to make constant strings identifiers.
> 
> (2) I believe we *could* optimize this case to notice that the two strings,
> though not pointer equal, are character equal, and do the constant folding
> anyway.
> 
> Probably not a big deal, but I thought I would share the idea.

It looks interesting. Do you have any static HTML data to reproduce this?
This is because I still cannot reproduce this assertion failure...
Opening facebook.com with my account (localized: JP) in GTK+ port MiniBrowser (Maybe it's different part). But the assertion does not fire...
I think this is due to the fact that I did not use facebook so much... (there are only few contents)

[WebKit Commit Bot]

Comment on attachment 262987 [details]
Patch

Clearing flags on attachment: 262987

Committed r190991: <http://trac.webkit.org/changeset/190991>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.