Bug 49976 - [Qt] Fix crashes in debug mode
Summary: [Qt] Fix crashes in debug mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-11-23 08:14 PST by Csaba Osztrogonác
Modified: 2010-12-10 02:20 PST (History)
7 users (show)

See Also:


Attachments
GDB backtrace (7.31 KB, text/plain)
2010-11-23 08:14 PST, Csaba Osztrogonác
no flags Details
fix (1.59 KB, patch)
2010-12-07 06:58 PST, Zoltan Herczeg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-11-23 08:14:27 PST
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.
Comment 1 Csaba Osztrogonác 2010-11-23 08:15:42 PST
I tried it with interpreter and JIT too, and I got same results,
so I think it is a general JavaScriptCore bug.
Comment 2 Csaba Osztrogonác 2010-11-23 08:17:27 PST
Have you got any idea what caused these crashes and how can we fix it?
Comment 3 Gavin Barraclough 2010-11-23 18:46:26 PST
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.
Comment 4 Robert Hogan 2010-11-24 11:57:17 PST
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.
Comment 5 Oliver Hunt 2010-11-28 19:11:27 PST
> 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.
Comment 6 Zoltan Herczeg 2010-11-30 07:25:38 PST
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.
Comment 7 Robert Hogan 2010-11-30 12:46:04 PST
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?
Comment 8 Oliver Hunt 2010-11-30 14:20:04 PST
(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?
Comment 9 Zoltan Herczeg 2010-12-05 23:44:06 PST
> 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.
Comment 10 Zoltan Herczeg 2010-12-05 23:51:10 PST
> 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.
Comment 11 Zoltan Herczeg 2010-12-06 00:48:29 PST
(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" ?
Comment 12 Oliver Hunt 2010-12-06 07:58:51 PST
(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)
Comment 13 Robert Hogan 2010-12-06 11:31:57 PST
Needs the patch at https://bugs.webkit.org/show_bug.cgi?id=50091 in order to recreate the problem
Comment 14 Zoltan Herczeg 2010-12-06 12:05:30 PST
> 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.
Comment 15 Zoltan Herczeg 2010-12-07 04:08:36 PST
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.
Comment 16 Zoltan Herczeg 2010-12-07 06:58:09 PST
Created attachment 75810 [details]
fix

Really tired of this bug...
Comment 17 WebKit Review Bot 2010-12-07 09:04:26 PST
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.
Comment 18 WebKit Review Bot 2010-12-07 10:05:10 PST
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.
Comment 19 Robert Hogan 2010-12-07 11:05:27 PST
Does this allow the tests unskipped in https://bugs.webkit.org/show_bug.cgi?id=37725 to still pass?
Comment 20 WebKit Review Bot 2010-12-07 11:06:05 PST
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.
Comment 21 WebKit Review Bot 2010-12-07 12:07:31 PST
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.
Comment 22 WebKit Review Bot 2010-12-07 21:38:02 PST
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.
Comment 23 Eric Seidel (no email) 2010-12-10 00:08:07 PST
Comment on attachment 75810 [details]
fix

OK.
Comment 24 WebKit Review Bot 2010-12-10 02:20:29 PST
Comment on attachment 75810 [details]
fix

Clearing flags on attachment: 75810

Committed r73699: <http://trac.webkit.org/changeset/73699>
Comment 25 WebKit Review Bot 2010-12-10 02:20:36 PST
All reviewed patches have been landed.  Closing bug.