Bug 212424 – REGRESSION(r260023): ITP sparse plist decoding fails silently

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

https://trac.webkit.org/changeset/260023 broke the build:
Regression in ITP sparse plist decoding.

This is an automatic bug report generated by webkitbot. If this bug
report was created because of a flaky test, please file a bug for the flaky
test (if we don't already have one on file) and dup this bug against that bug
so that we can track how often these flaky tests fail.

[Yusuke Suzuki]

Created attachment 400358 [details]
REVERT of r260023

Any committer can land this patch automatically by marking it commit-queue+.  The commit-queue will build and test the patch before landing to ensure that the revert will be successful.  This process takes approximately 15 minutes.

If you would like to land the revert faster, you can use the following command:

  webkit-patch land-attachment ATTACHMENT_ID

where ATTACHMENT_ID is the ID of this attachment.

[David Kilzer (:ddkilzer)]

<rdar://problem/63683055>

[David Kilzer (:ddkilzer)]

*** Bug 212422 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

*** Bug 212423 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

Looks like the revert patch needs to be fixed up.  Working on that now.

[David Kilzer (:ddkilzer)]

Created attachment 400367 [details]
Changes required  on top of revert

Changes necessary on top of revert when rolling out r260023.

[David Kilzer (:ddkilzer)]

Created attachment 400370 [details]
Patch v2

[Alex Christensen]

Comment on attachment 400370 [details]
Patch v2

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

> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)

Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?

[John Wilander]

Comment on attachment 400370 [details]
Patch v2

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

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
>> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)
> 
> Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?

That would work too. Or it could return an optional boolean where the tristate is true == found and decoded a value, nullopt == didn't find a value, false == found a value but failed to decode it.

Yet another option is to return a bool but create empty instances for false in ResourceLoadStatistics::decode() instead of bailing out.

[David Kilzer (:ddkilzer)]

Comment on attachment 400370 [details]
Patch v2

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

>>> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
>>> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)
>> 
>> Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?
> 
> That would work too. Or it could return an optional boolean where the tristate is true == found and decoded a value, nullopt == didn't find a value, false == found a value but failed to decode it.
> 
> Yet another option is to return a bool but create empty instances for false in ResourceLoadStatistics::decode() instead of bailing out.

Now is not the time to redesign the code.  We're trying to do a rollout.

I went with a third (hybrid) option that changes decodeHashCountedSet() and decodeHashSet() to return the `bool` value from KeyedDecoder::decodeObjects(), which fixes the compiler warning but leaves the return value of these static methods unchecked.  Semantically, this is no worse than what the patch was doing before—we just pass the bool up to a different function to ignore, and assume the caller knows what it's doing.

[David Kilzer (:ddkilzer)]

Created attachment 400389 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Comment on attachment 400370 [details]
Patch v2

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

>>>> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
>>>> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)
>>> 
>>> Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?
>> 
>> That would work too. Or it could return an optional boolean where the tristate is true == found and decoded a value, nullopt == didn't find a value, false == found a value but failed to decode it.
>> 
>> Yet another option is to return a bool but create empty instances for false in ResourceLoadStatistics::decode() instead of bailing out.
> 
> Now is not the time to redesign the code.  We're trying to do a rollout.
> 
> I went with a third (hybrid) option that changes decodeHashCountedSet() and decodeHashSet() to return the `bool` value from KeyedDecoder::decodeObjects(), which fixes the compiler warning but leaves the return value of these static methods unchecked.  Semantically, this is no worse than what the patch was doing before—we just pass the bool up to a different function to ignore, and assume the caller knows what it's doing.

Could also add a IGNORE_CLANG_WARNINGS_BEGIN("unused-result")/IGNORE_CLANG_WARNINGS_END around the KeyedDecoder::decodeObjects() function calls and change decodeHashCountedSet() and decodeHashSet() back to void functions.  Maybe that's a bit more self-documenting.

[David Kilzer (:ddkilzer)]

Created attachment 400394 [details]
Patch v4

[John Wilander]

Comment on attachment 400394 [details]
Patch v4

Yes, I like this version better. Self-documenting, as you say.

[David Kilzer (:ddkilzer)]

Comment on attachment 400389 [details]
Patch v3

Marking obsolete in order to take Patch v4.  Thanks for the review, Alex.

[David Kilzer (:ddkilzer)]

Waiting for EWS before marking cq+.

[David Kilzer (:ddkilzer)]

Comment on attachment 400394 [details]
Patch v4

Windows layout test failures appear to be unrelated to this change, so marking cq+.

[EWS]

Committed r262228: <https://trac.webkit.org/changeset/262228>

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