Bug 198766 – Add assertions to help debug crash at WebKit::HistoryEntryDataEncoder::operator<<

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 371874 [details]
Patch

[Sihui Liu]

Created attachment 371879 [details]
Patch

[Sihui Liu]

Created attachment 371968 [details]
Patch

[Geoffrey Garen]

Comment on attachment 371968 [details]
Patch

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

r=me

> Source/WebKit/Shared/SessionState.h:116
> +    bool isDestructed { false };
> +    ~FrameState() { isDestructed = true; }

This probably deserves a comment that it's only here to help debug an undiagnosed bug, with a Bugzilla or Radar link.

[Sihui Liu]

Created attachment 371990 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 371990 [details]
Patch for landing

Clearing flags on attachment: 371990

Committed r246382: <https://trac.webkit.org/changeset/246382>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51686085>

[Darin Adler]

Comment on attachment 371990 [details]
Patch for landing

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

> Source/WebKit/UIProcess/API/C/WKPage.cpp:485
> +    auto data = encodeLegacySessionState(sessionState);
>      if (shouldReturnData)
> -        return toAPI(encodeLegacySessionState(sessionState).leakRef());
> +        return toAPI(data.leakRef());

This isn’t adding an assertion. This is refactoring that causes us to do additional work when shouldReturnData is false. Nothing in the change log makes it clear why this is a desirable change.

[Sihui Liu]

Comment on attachment 371990 [details]
Patch for landing

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

>> Source/WebKit/UIProcess/API/C/WKPage.cpp:485
>> +        return toAPI(data.leakRef());
> 
> This isn’t adding an assertion. This is refactoring that causes us to do additional work when shouldReturnData is false. Nothing in the change log makes it clear why this is a desirable change.

Right, add some explaining here for reference.

Previously the crash traces tell us:
WKPageCopySessionState returns WKSessionStateRef => Some pointer casts by user => WKSessionStateCopyData(WKSessionStateRef) -> encodeLegacySessionState(sessionState) [Crash]

Now:
WKPageCopySessionState [try encodeLegacySessionState(sessionState)] returns WKSessionStateRef => pointer casts => WKSessionStateCopyData(WKSessionStateRef) -> encodeLegacySessionState(sessionState)

If we start to see crashes in WKPageCopySessionState after this change, we can confirm sessionState is bad before the casts.