Summary: | Using --recordGCPauseTimes=true causes a crash in jsc. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tamas Gergely <tgergely.u-szeged> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | NEW --- | ||||||||||||||||
Severity: | Normal | CC: | buildbot, bunhere, cdumez, commit-queue, ggaren, gyuyoung.kim, rniwa, sergio | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Tamas Gergely
2014-06-06 03:03:42 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.
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.
Created attachment 233222 [details]
patch
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 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? > ERROR: Source/JavaScriptCore/jsc.cpp:1251: Missing space before { [whitespace/braces] [5]
> Total errors found: 1 in 3 files
Please fix this style issue.
(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(). Created attachment 233306 [details]
patch
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. (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. Created attachment 233424 [details]
patch
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 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
Created attachment 234152 [details]
updated patch
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.
|