Bug 133572 – Using --recordGCPauseTimes=true causes a crash in jsc.

• Component: New Bugs
• Status: NEW

[Tamas Gergely]

The --recordGCPauseTimes=true command line option causes a crash in jsc, due to too early initialization.

[Tamas Gergely]

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.

[WebKit Commit Bot]

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.

[Tamas Gergely]

Created attachment 233222 [details]
patch

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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?

[Geoffrey Garen]

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

Please fix this style issue.

[Tamas Gergely]

(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().

[Tamas Gergely]

Created attachment 233306 [details]
patch

[Geoffrey Garen]

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.

[Tamas Gergely]

(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.

[Tamas Gergely]

Created attachment 233424 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Tamas Gergely]

Created attachment 234152 [details]
updated patch

[Michael Catanzaro]

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.