Summary: | DFG::freezeFragile should register the frozen value's structure | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | barraclough, ggaren, mark.lam, mhahnenb, mmirman, msaboff, oliver, sam | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 133426, 135750 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Filip Pizlo
2014-08-18 17:53:18 PDT
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. Created attachment 255873 [details]
the patch
Comment on attachment 255873 [details]
the patch
r=me
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. (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. (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... Created attachment 255979 [details]
the patch
Comment on attachment 255979 [details]
the patch
r=me
Landed in http://trac.webkit.org/changeset/186215 |