Bug 49976 – [Qt] Fix crashes in debug mode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

Created attachment 74668 [details]
GDB backtrace

On trunk 8 tests crash in debug mode: (r72592)
dom/html/level1/core/hc_attrappendchild3.html
dom/xhtml/level2/html/table07.xhtml
dom/xhtml/level2/html/table22.xhtml
fast/js/sputnik/Conformance/15_Native_Objects/15.3_Function/15.3.3/S15.3.3_A2_T2.html
fast/js/sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A1.1_T1.html
fast/js/sputnik/Conformance/15_Native_Objects/15.7_Number/15.7.3/15.7.3.5_Number.NEGATIVE_INFINITY/S15.7.3.5_A1.html
fast/js/sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.2/15.10.2.12_CharacterClassEscape/S15.10.2.12_A6_T2.html
ietestcenter/Javascript/15.4.4.15-5-8.html

These tests don't crash if we run only one test, but we can reproduce the crashes.

for example I tried with the following tests:
dom/xhtml/level2/html/object11.xhtml
dom/xhtml/level2/html/object12.xhtml
dom/xhtml/level2/html/object13.xhtml
dom/xhtml/level2/html/object14.xhtml
dom/xhtml/level2/html/object15.xhtml
dom/xhtml/level2/html/table01.xhtml
dom/xhtml/level2/html/table02.xhtml
dom/xhtml/level2/html/table03.xhtml
dom/xhtml/level2/html/table04.xhtml
dom/xhtml/level2/html/table06.xhtml
dom/xhtml/level2/html/table07.xhtml

GDB backtrace is attached.

[Csaba Osztrogonác]

I tried it with interpreter and JIT too, and I got same results,
so I think it is a general JavaScriptCore bug.

[Csaba Osztrogonác]

Have you got any idea what caused these crashes and how can we fix it?

[Gavin Barraclough]

If I'm reading the backtrace correctly, it looks like this assert is failing:

> result == CallTypeNone || value.isValidCallee()

Which means value is not a valid callee.  We now require all callable objects in JSC (i.e. host functions) to be derived from JSObjectWithGlobalObject, which is what isValidCallee() tests.  Objects created via the JSC API should all derive from JSObjectWithGlobalObject (well, bar global objects created via the API, but they'll still be okay – isValidCallee() actually checks the contents of anonymous slot 0, GOs will pass this check too).

So my guess would be something like, that the Qt DRT is exposing some host functions to JS by directly deriving from a JSObject type that is not a JSObjectWithGlobalObject, and directly implementing the getCallData() callback itself.  If so, a fix would be to use the NativeFunctionWrapper type when wrapping C functions.

My next step in debugging this would be to see what function was being called when this ASSERT fired.

Hope this helps.
G.

[Robert Hogan]

This is due to a problem with m_cacheableBindingRootObject in ScriptController. The call that is causing the crash is on LayoutTestController which is a runtime object and is a JSObjectWithGlobalObject. 

Darin predicted this in https://bugs.webkit.org/show_bug.cgi?id=48758#c13 unfortunately.

This particular crash seems to be due to a stale reference to the globalObject in the runtime object. This happens because m_cacheableBindingRootObject persists between page loads. The globalObject associated with the root object is updated between page loads since bug 48758. However there is still a reference to the old global object in the runtime object associated with the root object's/JSC object's instance. So it looks like that needs to be updated as well.

I'm not sure why this is only a problem on debug builds.

[Oliver Hunt]

> I'm not sure why this is only a problem on debug builds.

I assume it's an assertion (eg. debug only), and so more-or-less deterministic in debug builds, whereas in release builds i suspect it works through luck.

[Zoltan Herczeg]

This is a Garbage Collector issue. The m_structure of QtRuntimeMetaMethod is also a JSValue (JSDOMWindow). This object is freed by GC during the execution of table07.xhtml. QtRuntimeMetaMethod is an InternalFunction. How an InternalFunction (JSObject descendant) should mark its asObject(this)->getAnonymousValue(0) ( == propertyStorage()[0] )? Would be good to know how QtRuntimeMetaMethod itself is marked.

[Robert Hogan]

Hi Zoltan,

Thanks for looking at this.

(In reply to comment #6)
> This is a Garbage Collector issue. The m_structure of QtRuntimeMetaMethod is also a JSValue (JSDOMWindow). This object is freed by GC during the execution of table07.xhtml. 

How can you tell? I'd like to know how I could have spotted this!

>QtRuntimeMetaMethod is an InternalFunction. How an InternalFunction (JSObject descendant) should mark its asObject(this)->getAnonymousValue(0) ( == propertyStorage()[0] )? Would be good to know how QtRuntimeMetaMethod itself is marked.

Would a RefPtr protect(value) not do it? If so, why not?

[Oliver Hunt]

(In reply to comment #6)
> This is a Garbage Collector issue. The m_structure of QtRuntimeMetaMethod is also a JSValue (JSDOMWindow). This object is freed by GC during the execution of table07.xhtml. QtRuntimeMetaMethod is an InternalFunction. How an InternalFunction (JSObject descendant) should mark its asObject(this)->getAnonymousValue(0) ( == propertyStorage()[0] )? Would be good to know how QtRuntimeMetaMethod itself is marked.

Structures aren't GC allocated, i assume you mean the global object reference?

[Zoltan Herczeg]

> Structures aren't GC allocated, i assume you mean the global object reference?

Yeah, I was not precise:

static PassRefPtr<Structure> createStructure(JSValue proto).
{
     return Structure::create(proto, TypeInfo(ObjectType, StructureFlags), AnonymousSlotCount);.
}

The "proto" object is freed.

[Zoltan Herczeg]

> How can you tell? I'd like to know how I could have spotted this!

Requires a lot of JSC internal knowledge. First, you should "feel" this is a GC issue, and know which object will be freed (but still referenced). Then you need to put a breakpoint where this object must be exists (i.e: its constructor), and execute a "watch *(int*)this" inside GDB. This put a write watchpoint to the vptr (virtual ptr) of an object, which never changes, except if the memory is overwritten. In case of GC, it overwrites this are during a sweep, since it maintains a chan list for free cells. And GDB will stops when this particular object is sweeped by GC.

> Would a RefPtr protect(value) not do it? If so, why not?

No, cells live an entiriely different life cycle than anny other objects in WebKit. The GC should tell which cells are referenced, and which are not.

[Zoltan Herczeg]

(In reply to comment #9)
> > Structures aren't GC allocated, i assume you mean the global object reference?

(In reply to comment #9)
> > Structures aren't GC allocated, i assume you mean the global object reference?
> 
> Yeah, I was not precise:
> 
> static PassRefPtr<Structure> createStructure(JSValue proto).
> {
>      return Structure::create(proto, TypeInfo(ObjectType, StructureFlags), AnonymousSlotCount);.
> }
> 
> The "proto" object is freed.

Ah hell, no. So you were right:

JSObjectWithGlobalObject::JSObjectWithGlobalObject (base class of InternalFunction)
  putAnonymousValue(GlobalObjectSlot, globalObject);

Hm, shall this class should mark its "globalObject" ?

[Oliver Hunt]

(In reply to comment #11)
> (In reply to comment #9)
> > > Structures aren't GC allocated, i assume you mean the global object reference?
> 
> (In reply to comment #9)
> > > Structures aren't GC allocated, i assume you mean the global object reference?
> > 
> > Yeah, I was not precise:
> > 
> > static PassRefPtr<Structure> createStructure(JSValue proto).
> > {
> >      return Structure::create(proto, TypeInfo(ObjectType, StructureFlags), AnonymousSlotCount);.
> > }
> > 
> > The "proto" object is freed.
> 
> Ah hell, no. So you were right:
> 
> JSObjectWithGlobalObject::JSObjectWithGlobalObject (base class of InternalFunction)
>   putAnonymousValue(GlobalObjectSlot, globalObject);
> 
> Hm, shall this class should mark its "globalObject" ?

Its global object should be marked through the base JSObject::markChildren method (which marks all of the properties slots, including anon. storage)

[Robert Hogan]

Needs the patch at https://bugs.webkit.org/show_bug.cgi?id=50091 in order to recreate the problem

[Zoltan Herczeg]

> Its global object should be marked through the base JSObject::markChildren method (which marks all of the properties slots, including anon. storage)

Looks reasonable. However, the InernalFunction object exits after GC (its vptr is still valid, thus it is not GC'ed) while the JSDOMWindow is invalid. How this can be happen? Seems black magic.

[Zoltan Herczeg]

Hopefully I know what is happening know. A new "feature" was introduced to the GarbageCollector (new at least for me), which defers the destruction of Cells until they reused (thus, immediate sweep is unneeded, and distributing the sweep time during the actual allocation). Thus, "dead" objects can still seems living, and that was confused me. All InternalFunctions allocated by QtClass are sweeped out, and the name/method hashmap cache becomes invalid.

Two possible solutions:
1) Something should somehow mark these objects. Is there an easy way to do this?

2) Drop this cache (this would follow the cacheless ObjectiveC implemetation)

Since these objects are probably only used by DumpRenderTree, I would vote to 2, but would be interested in your opinion.

[Zoltan Herczeg]

Created attachment 75810 [details]
fix

Really tired of this bug...

[WebKit Review Bot]

Attachment 75810 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75810 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Hogan]

Does this allow the tests unskipped in https://bugs.webkit.org/show_bug.cgi?id=37725 to still pass?

[WebKit Review Bot]

Attachment 75810 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75810 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75810 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 75810 [details]
fix

OK.

[WebKit Review Bot]

Comment on attachment 75810 [details]
fix

Clearing flags on attachment: 75810

Committed r73699: <http://trac.webkit.org/changeset/73699>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.