Bug 133572 - Using --recordGCPauseTimes=true causes a crash in jsc.
Summary: Using --recordGCPauseTimes=true causes a crash in jsc.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-06 03:03 PDT by Tamas Gergely
Modified: 2016-09-17 07:11 PDT (History)
8 users (show)

See Also:


Attachments
patch proposal (2.32 KB, patch)
2014-06-06 03:15 PDT, Tamas Gergely
tgergely.u-szeged: commit-queue-
Details | Formatted Diff | Diff
patch (2.99 KB, patch)
2014-06-17 01:39 PDT, Tamas Gergely
no flags Details | Formatted Diff | Diff
patch (2.95 KB, patch)
2014-06-18 06:42 PDT, Tamas Gergely
no flags Details | Formatted Diff | Diff
patch (3.95 KB, patch)
2014-06-20 05:58 PDT, Tamas Gergely
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (727.71 KB, application/zip)
2014-06-20 11:50 PDT, Build Bot
no flags Details
updated patch (3.99 KB, patch)
2014-07-01 02:45 PDT, Tamas Gergely
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tamas Gergely 2014-06-06 03:03:42 PDT
The --recordGCPauseTimes=true command line option causes a crash in jsc, due to too early initialization.
Comment 1 Tamas Gergely 2014-06-06 03:15:03 PDT
Created attachment 232607 [details]
patch proposal

I moved the call of HeapStatistics::initialization() out of the initializeThreading() function, and added a call in the jscmain. I guess HeapStatistics::initialization() is not relevant at other initializeThreading() calls, but I'm not sure.
Comment 2 WebKit Commit Bot 2014-06-06 03:17:05 PDT
Attachment 232607 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:1250:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tamas Gergely 2014-06-17 01:39:23 PDT
Created attachment 233222 [details]
patch
Comment 4 WebKit Commit Bot 2014-06-17 01:40:19 PDT
Attachment 233222 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:1251:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Geoffrey Garen 2014-06-17 11:15:20 PDT
Comment on attachment 233222 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233222&action=review

> Source/JavaScriptCore/ChangeLog:12
> +        This is because JSC::initializeThreading() initializes HeapStatistics
> +        before Options values get filled from the command line arguments, while
> +        this initialization is dependent on the options.

I see a call to Options::recordGCPauseTimes(). Why is that not sufficient to fill in Options values from command line arguments?
Comment 6 Geoffrey Garen 2014-06-17 11:15:36 PDT
> ERROR: Source/JavaScriptCore/jsc.cpp:1251:  Missing space before {  [whitespace/braces] [5]
> Total errors found: 1 in 3 files

Please fix this style issue.
Comment 7 Tamas Gergely 2014-06-18 06:21:43 PDT
(In reply to comment #5)
> (From update of attachment 233222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233222&action=review
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        This is because JSC::initializeThreading() initializes HeapStatistics
> > +        before Options values get filled from the command line arguments, while
> > +        this initialization is dependent on the options.
> 
> I see a call to Options::recordGCPauseTimes(). Why is that not sufficient to fill in Options values from command line arguments?

Because Options::initialize() in the previous line initializes the options to their default values *without checking the command line*. The call to Options::recordGCPauseTimes() returns false here as it is the default value, so the HeapStatistics::initialize() is not called. Command line arguments are processed by the construction of a CommandLine object in the beginning of the jscmain(), but it was too late: we missed the call of HeapStatistics::initialize().
Comment 8 Tamas Gergely 2014-06-18 06:42:32 PDT
Created attachment 233306 [details]
patch
Comment 9 Geoffrey Garen 2014-06-18 11:15:33 PDT
Comment on attachment 233306 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233306&action=review

> Source/JavaScriptCore/jsc.cpp:1251
> +    static std::once_flag optionDependentInitOnceFlag;
> +    std::call_once(optionDependentInitOnceFlag, [] {

Since this is main, there's no need for call_once-style thread-safety.

> Source/JavaScriptCore/runtime/InitializeThreading.cpp:-62
> -        if (Options::recordGCPauseTimes())
> -            HeapStatistics::initialize();

I don't think we want to remove this from the generic initialization path, because that would make it not work in WebKit.
Comment 10 Tamas Gergely 2014-06-20 05:50:41 PDT
(In reply to comment #9)
> (From update of attachment 233306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233306&action=review
> 
> > Source/JavaScriptCore/jsc.cpp:1251
> > +    static std::once_flag optionDependentInitOnceFlag;
> > +    std::call_once(optionDependentInitOnceFlag, [] {
> 
> Since this is main, there's no need for call_once-style thread-safety.
> 
> > Source/JavaScriptCore/runtime/InitializeThreading.cpp:-62
> > -        if (Options::recordGCPauseTimes())
> > -            HeapStatistics::initialize();
> 
> I don't think we want to remove this from the generic initialization path, because that would make it not work in WebKit.

Ok. Then I'll try another solution: make HeapStatistics::initialize() smarter to handle multiple calls. Should we handle the rare situation when the option is set from environment variable and cleared from command line? If not, we will create two unused Vectors on the heap in this situation.
Comment 11 Tamas Gergely 2014-06-20 05:58:35 PDT
Created attachment 233424 [details]
patch
Comment 12 Build Bot 2014-06-20 11:50:02 PDT
Comment on attachment 233424 [details]
patch

Attachment 233424 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4591621744099328

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html
Comment 13 Build Bot 2014-06-20 11:50:04 PDT
Created attachment 233439 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 14 Tamas Gergely 2014-07-01 02:45:12 PDT
Created attachment 234152 [details]
updated patch
Comment 15 Michael Catanzaro 2016-09-17 07:11:33 PDT
Comment on attachment 234152 [details]
updated patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.