Bug 154930 – Add state dumping facility

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Keith Rollin]

Add support for dumping state information when there's a crash or other type of fault.

[Radar WebKit Bug Importer]

<rdar://problem/24939135>

[Keith Rollin]

Created attachment 273345 [details]
Patch

[WebKit Commit Bot]

Attachment 273345 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:217:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 273345 [details]
Patch

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

Check if sam or anders can do a proper review for this one.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:938
> +    std::chrono::system_clock::time_point lastPageLoadTime() const { return m_lastPageLoadTime; }

The name "lastPageLoadTime" is a bit confusing. I would prefer something like "loadCommitTime".

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:232
> +            for (auto& page : this->m_pageMap.values()) {

do we need the explicit this-> ?

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:284
> +    (void) os_state_add_handler(queue, block);

Do we really need this (void) ?

[Anders Carlsson]

Comment on attachment 273345 [details]
Patch

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

Patch looks very good overall!

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:217
> +    os_state_block_t block = ^(os_state_hints_t hints) {

I think you could just pass the block directly to os_state_add_handler here instead of declaring it and then calling os_state_add_handler.

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:229
> +            auto stateDict = [[[NSMutableDictionary alloc] init] autorelease];

Instead of using autorelease here, you can use a RetainPtr. Something like

auto stateDictionary = adoptNS([[NSMutabledictionary alloc] init]);

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:231
> +            auto pageLoadTimes = [[[NSMutableArray alloc] init] autorelease];

Same thing here.

>> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:232
>> +            for (auto& page : this->m_pageMap.values()) {
> 
> do we need the explicit this-> ?

I don't think so.

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:238
> +                time_t t = std::chrono::system_clock::to_time_t(page->lastPageLoadTime());
> +                NSDate* asDate = [NSDate dateWithTimeIntervalSince1970:t];
> +                [pageLoadTimes addObject:asDate];

I'd just fold t into the date here. I'd call "asDate" just date.

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:257
> +            NSError* error;

Error should be initialized to nil here (I think).

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:262
> +            NSData* data = [NSPropertyListSerialization
> +                dataWithPropertyList: stateDict
> +                format: NSPropertyListBinaryFormat_v1_0
> +                options: 0
> +                error: &error];

We don't use this Objective-C method formatting anymore; these all go on one line.

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:265
> +                LOG_ALWAYS_ERROR("Error serializing process state information: %{public}@", error);

I don't think it's useful to log the error here - the only time we'd fail to serialize the dictionary is if it contains objects that can't be serialized. Since we control the dictionary, I think you can just ASSERT(data) here.

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:275
> +                // os_state->osd_decoder = ...;

Is this a FIXME comment? If so, it's better to say something like:

// FIXME: Fill in osd_decoder with ...

[Keith Rollin]

Created attachment 273368 [details]
Patch

[Anders Carlsson]

Comment on attachment 273368 [details]
Patch

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

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:243
> +            if (pageLoadTimes.get().count)

Our coding style is to not use .get().count (too many dots) - instead we use [pageLoadTimes count].

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:244
> +                stateDict.get()[@"Page Load Times"] = pageLoadTimes.get();

Same thing here - I"d just use [pageLoadTimes setObject: forKey:].

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:251
> +            if (!stateDict.get().count)

And same thing here.

[Keith Rollin]

Created attachment 273490 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 273490 [details]
Patch

Clearing flags on attachment: 273490

Committed r197901: <http://trac.webkit.org/changeset/197901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.