Bug 136055 – DFG::freezeFragile should register the frozen value's structure

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

...

[Filip Pizlo]

I believe that we previously did not do this because of the suspicion that it might be worthwhile to sometimes track a value without tracking its structure.  But right now all users of freeze() will eventually register the value's structure, except in cases where they obviously should but they obviously don't.  That causes bugs.  It would be easier to just make freeze() always register the structure and then not have any doubts about whose responsibility it is.

[Filip Pizlo]

Created attachment 255873 [details]
the patch

[Geoffrey Garen]

Comment on attachment 255873 [details]
the patch

r=me

[Mark Lam]

Comment on attachment 255873 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:55
>          for (FrozenValue* value : m_graph.m_frozenValues)
> -            registerStructure(value->structure());
> +            m_graph.assertIsRegistered(value->structure());

According to Graph::freezeFragile() and FrozenValue::freeze(), we can freeze non-cell values.  In those cases, value->structure() is a nullptr.  Is this assertion valid without a nullcheck on value->structure() first?  In Graph::assertIsRegistered(), it looks like it will deref the passed in structure without any checks.

[Filip Pizlo]

(In reply to comment #4)
> Comment on attachment 255873 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255873&action=review
> 
> > Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:55
> >          for (FrozenValue* value : m_graph.m_frozenValues)
> > -            registerStructure(value->structure());
> > +            m_graph.assertIsRegistered(value->structure());
> 
> According to Graph::freezeFragile() and FrozenValue::freeze(), we can freeze
> non-cell values.  In those cases, value->structure() is a nullptr.  Is this
> assertion valid without a nullcheck on value->structure() first?  In
> Graph::assertIsRegistered(), it looks like it will deref the passed in
> structure without any checks.

Wow, good catch!  It turns out that this works because assertIsRegistered() returns early before the end of the structure registration phase.  So this is just a broken no-op!  I'll fix and post a new patch.

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 255873 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255873&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:55
> > >          for (FrozenValue* value : m_graph.m_frozenValues)
> > > -            registerStructure(value->structure());
> > > +            m_graph.assertIsRegistered(value->structure());
> > 
> > According to Graph::freezeFragile() and FrozenValue::freeze(), we can freeze
> > non-cell values.  In those cases, value->structure() is a nullptr.  Is this
> > assertion valid without a nullcheck on value->structure() first?  In
> > Graph::assertIsRegistered(), it looks like it will deref the passed in
> > structure without any checks.
> 
> Wow, good catch!  It turns out that this works because assertIsRegistered()
> returns early before the end of the structure registration phase.  So this
> is just a broken no-op!  I'll fix and post a new patch.

And this revealed a bug.  freezeFragile() was loading the structure and registering it, but then the act of freezing would load the structure a second time.  That was racy.  The fix is to load the structure only once.

Posting new patch now...

[Filip Pizlo]

Created attachment 255979 [details]
the patch

[Mark Lam]

Comment on attachment 255979 [details]
the patch

r=me

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/186215