Bug 20914 - [QT] JavaScriptCore segmentation fault
Summary: [QT] JavaScriptCore segmentation fault
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 20974
  Show dependency treegraph
 
Reported: 2008-09-18 05:49 PDT by Peter Gal
Modified: 2008-09-22 07:29 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (1.43 KB, patch)
2008-09-18 05:51 PDT, Peter Gal
timothy: review+
Details | Formatted Diff | Diff
typo fixed patch (1.43 KB, patch)
2008-09-19 05:28 PDT, Peter Gal
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Gal 2008-09-18 05:49:04 PDT
After r36541 when running JavaScriptCore it exits with segmentation fault. The problem seems to be the QCoreApplication::instance() call wich returns null.
Comment 1 Peter Gal 2008-09-18 05:51:05 PDT
Created attachment 23528 [details]
proposed patch
Comment 2 Peter Gal 2008-09-19 05:28:52 PDT
Created attachment 23563 [details]
typo fixed patch

The previous patch had a typo, which made a header to be skipped if the platform is a MAC. It was tested only on linux/qt so this is why I missed it.
Comment 3 Eric Seidel (no email) 2008-09-21 14:47:57 PDT
Comment on attachment 23563 [details]
typo fixed patch

I'm going to flip the meaning of the second #if when I land, so that  it's more clear the code is only used for DARWIN.
Comment 4 Eric Seidel (no email) 2008-09-21 14:54:16 PDT
Landed as r36745, thanks for the patch!
Comment 5 Alexey Proskuryakov 2008-09-22 00:58:03 PDT
I don't see why this fix is correct - won't this just keep crashing on Darwin, if it did on other platforms?

I think you can just use "mainThreadIdentifier = currentThread();" everywhere (although I'm not sure why the variable is needed on Darwin at all).
Comment 6 Peter Gal 2008-09-22 01:18:17 PDT
I couldn't tested it on any other platform than linux.
I looked webkit's buildbot and there the mac version didn't show any errors while the linux version clearly crashed. This is the main reason why I think this patch is ok.


Don't really now why was it changed in r36541, but I did't wanted to remove that code, maybe there will be some future plans whit it?
Comment 7 Tor Arne Vestbø 2008-09-22 01:38:14 PDT
Yes, kjs will still crash on Darwin.

If I understood you correct Alexey, JSC::initializeThreading() can be called from non-main threads on Darwin, so I used QCoreApplication to get the main thread instead of currentThread(), so that mainThreadIdentifier will always be pointing to the main thread no matter which thread the init was called from.

What I didn't foresee was that the jsc binary does not init QCoreApplication. I'll look into it.

Thanks for the notice :) 
Comment 8 Tor Arne Vestbø 2008-09-22 06:30:25 PDT
Landed in r36763
Comment 9 Alexey Proskuryakov 2008-09-22 07:23:56 PDT
(In reply to comment #7)
> If I understood you correct Alexey, JSC::initializeThreading() can be called
> from non-main threads on Darwin

At the moment, the contract is that JSC::initializeThreading() should be called from the main thread. One exception is Mac OS X JavaScriptCore API clients, and that's because some of these shipped before we put this in place.

So, for the Qt port JSC::initializeThreading() should be called from the main thread, even on Darwin. You are free to see this as an unfortunate limitation, and lift it some day, of course.

I'm not sure that the workaround with calling QCoreApplication is so great - besides our own test shell, there may be other command line apps or daemons that want to use JSC, and creating a full QCoreApplication my be inappropriate for some. But at least it gets us going for now.