| Summary: | [Qt] Fix crashes in debug mode | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | barraclough, darin, loki, oliver, robert, webkit.review.bot, zherczeg | ||||||
| Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Csaba Osztrogonác
2010-11-23 08:14:27 PST
I tried it with interpreter and JIT too, and I got same results, so I think it is a general JavaScriptCore bug. Have you got any idea what caused these crashes and how can we fix it? 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.
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.
> 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.
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. 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? (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? > 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.
> 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. (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" ? (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) Needs the patch at https://bugs.webkit.org/show_bug.cgi?id=50091 in order to recreate the problem
> 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.
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. Created attachment 75810 [details]
fix
Really tired of this bug...
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.
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.
Does this allow the tests unskipped in https://bugs.webkit.org/show_bug.cgi?id=37725 to still pass? 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.
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.
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 on attachment 75810 [details]
fix
OK.
Comment on attachment 75810 [details] fix Clearing flags on attachment: 75810 Committed r73699: <http://trac.webkit.org/changeset/73699> All reviewed patches have been landed. Closing bug. |