Bug 24823 - <rdar://problem/6603167> Crash in WebKit!JSC::JSGlobalObject::resetPrototype during Stress test (#3 & #7 WER crashes for Safari 4 Beta)
Summary: <rdar://problem/6603167> Crash in WebKit!JSC::JSGlobalObject::resetPrototype ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-25 16:10 PDT by Geoffrey Garen
Modified: 2009-03-26 13:29 PDT (History)
1 user (show)

See Also:


Attachments
patch (3.31 KB, patch)
2009-03-25 16:10 PDT, Geoffrey Garen
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2009-03-25 16:10:20 PDT
Patch coming.
Comment 1 Geoffrey Garen 2009-03-25 16:10:38 PDT
Created attachment 28951 [details]
patch
Comment 2 Oliver Hunt 2009-03-25 16:16:02 PDT
Comment on attachment 28951 [details]
patch

r=me
Comment 3 Darin Adler 2009-03-25 16:17:20 PDT
Comment on attachment 28951 [details]
patch

> +    ProtectedPtr<JSDOMWindowPrototype> prototype = new JSDOMWindowPrototype(prototypeStructure.release());

Needs a comment. We so rarely use ProtectedPtr that I think it's a special event when we do use it. Something like "protect the pointer until the structure is inside a constructed xxx object".

> +    ProtectedPtr<JSWorkerContextPrototype> prototype = new (m_globalData.get()) JSWorkerContextPrototype(prototypeStructure.release());

Same.

r=me
Comment 4 Geoffrey Garen 2009-03-25 19:12:39 PDT
Committed revision 41997.
Comment 5 Alexey Proskuryakov 2009-03-25 22:25:19 PDT
Why do we need to use ProtectedPtr, cannot it be a plain pointer on the stack?

ProtectedPtr is evil, it uses lock, and thus increases thread contention.
Comment 6 Geoffrey Garen 2009-03-26 11:50:39 PDT
There are GC solutions other than ProtectedPtr. For example, we could do something similar to what we do for JSGlobalData::exception.

I'd prefer to avoid a plain stack pointer because ensuring, in a Release build, that the pointer is visible on the stack during GC is tricky. You could try using the "volatile" keyword, but its behavior, especially with regard to automatic local variables, is notoriously surprising, and it guarantees only  that the pointer will be written to memory, but not that it will not later be overwritten before GC.

> ProtectedPtr is evil, it uses lock, and thus increases thread contention.

How evil is it? Does it show up as a problem on benchmarks?

From the comments in Collector.cpp, I see this explanation for why ProtectedPtr uses a lock: "WebCore violates this contract in Database code, which calls gcUnprotect from a secondary thread."

If the lock is truly evil, it seems easy enough to fix the one error in WebCore that necessitates its use. I recorded this design failure in <rdar://problem/5751979> almost a year ago. The only reason it wasn't fixed was that Maciej wasn't convinced that it was a user-visible problem.
Comment 7 Alexey Proskuryakov 2009-03-26 13:29:28 PDT
(In reply to comment #6)
> How evil is it? Does it show up as a problem on benchmarks?

Probably not this one, but the one in ScriptValue certainly does (Oliver was going to talk to you about that recently). I'm not sure if fixing Database code is easy - at least, it wasn't easy for me when I tried.

Also, gc protection causes bug 21260, which is completely unrelated, but a sign of evilness, too.