Bug 222587 – Validate documentState of FrameState when setting and getting it

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

[Sihui Liu]

...

[Sihui Liu]

Created attachment 421907 [details]
Patch

[Sihui Liu]

Created attachment 421945 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Chris Dumez]

Comment on attachment 421945 [details]
Patch

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

> Source/WebKit/Shared/SessionState.cpp:282
> +        StringView(stateString).upconvertedCharacters();

Maybe a comment to explain why this is validating? It is not clear to me at least.

> Source/WebKit/Shared/SessionState.h:90
> +    Vector<String> documentState() const { return  m_documentState; }

should return a `const Vector<String>&`

> Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:310
>          frameState.documentState.reserveInitialCapacity(documentStateLength);

Looks like this line should be dropped?

> Source/WebKit/UIProcess/mac/LegacySessionStateCoding.cpp:907
> +            frameState.setDocumentState(documentState, FrameState::ShouldValidate::Yes);

Decoding failed here. So what's the point of this?

[Sihui Liu]

Created attachment 421984 [details]
Patch

[Sihui Liu]

Comment on attachment 421945 [details]
Patch

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

>> Source/WebKit/Shared/SessionState.cpp:282
>> +        StringView(stateString).upconvertedCharacters();
> 
> Maybe a comment to explain why this is validating? It is not clear to me at least.

I just added the crashed function here, it seems it failed when traversing the characters. Will add a comment.

>> Source/WebKit/Shared/SessionState.h:90
>> +    Vector<String> documentState() const { return  m_documentState; }
> 
> should return a `const Vector<String>&`

Okay.

>> Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:310
>>          frameState.documentState.reserveInitialCapacity(documentStateLength);
> 
> Looks like this line should be dropped?

Right, good catch, will remove.

>> Source/WebKit/UIProcess/mac/LegacySessionStateCoding.cpp:907
>> +            frameState.setDocumentState(documentState, FrameState::ShouldValidate::Yes);
> 
> Decoding failed here. So what's the point of this?

The old behavior is updating documentState even it fails, so I just followed that.

[Geoffrey Garen]

Comment on attachment 421984 [details]
Patch

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

> Source/WebKit/Shared/SessionState.cpp:289
> +            auto character = characters8[i];
> +            UNUSED_PARAM(character);

I believe the compiler is smart enough to notice that 'character' is unused, and skip the load from characters8 altogether. That will turn this validation into a no-op.

You should be able to force a load by doing "volatile LChar character = characters8[I]".

But perhaps a better idea would be to RELEASE_ASSERT(isLatin1(character)). (LChar is a 7-bit Latin1 encoding.) That's slightly more validation, and honestly every time I've tried to use the 'volatile' keyword I've done it wrong.

[Sihui Liu]

Created attachment 422001 [details]
Patch

[Sihui Liu]

Created attachment 422274 [details]
Patch

[Geoffrey Garen]

Comment on attachment 422274 [details]
Patch

r=me

Based on the analysis of other crash logs, maybe we should add a main thread RELEASE_ASSERT too.

[Sihui Liu]

Created attachment 422293 [details]
Patch for landing

[Sihui Liu]

Created attachment 422297 [details]
Patch for landing

[EWS]

Patch 422293 does not build

[EWS]

Committed r273929: <https://commits.webkit.org/r273929>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422297 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75063667>