Bug 221848 – REGRESSION (r268421): TestWebKitAPI.WebKit.PreferenceChanges* tests are flaky failures

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

REGRESSION (r268421): TestWebKitAPI.WebKit.PreferenceChanges* tests are flaky failures

TestWebKitAPI.WebKit.PreferenceChangesDate
TestWebKitAPI.WebKit.PreferenceChangesArray
TestWebKitAPI.WebKit.PreferenceChangesData
TestWebKitAPI.WebKit.PreferenceChangesNil

[Ryan Haddad]

These tests were disabled in https://trac.webkit.org/changeset/270510/webkit

[Ryan Haddad]

<rdar://problem/70966379>

[Ryan Haddad]

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

[Ryan Haddad]

I also disabled TestWebKitAPI.WebKit.GlobalPreferenceChangesUsingDefaultsWrite in https://commits.webkit.org/234051@main

[Per Arne Vollan]

Created attachment 443038 [details]
Patch

[Brent Fulgham]

Comment on attachment 443038 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/PreferenceChanges.mm:191
> +        TestWebKitAPI::Util::spinRunLoop();

This will potentially slow down the process. Is it possible to wait until a sync has happened (perhaps using an event or completion handler)?

> Tools/TestWebKitAPI/Tests/WebKit/PreferenceChanges.mm:223
> +    for (;;) {

It seems like this could hang the test.

[Per Arne Vollan]

Created attachment 443103 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #6)
> Comment on attachment 443038 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443038&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit/PreferenceChanges.mm:191
> > +        TestWebKitAPI::Util::spinRunLoop();
> 
> This will potentially slow down the process. Is it possible to wait until a
> sync has happened (perhaps using an event or completion handler)?
> 

That is a good point. I think we for example could post a notification in the WebContent process when a preference is set, which TestWebKitAPI could then subscribe to. There is some ambiguity to this approach, however, since we're not using WebKit API to set the preference, but using NSUserDefaults directly. This means it is not trivial to match the preference change made by TestWebKitAPI with the corresponding notification, since the preference change could also have come from elsewhere. Due to this ambiguity, I did not go down this road, but added a sleep interval to the loop in order to avoid using too much CPU. Would you be OK with that approach?

> > Tools/TestWebKitAPI/Tests/WebKit/PreferenceChanges.mm:223
> > +    for (;;) {
> 
> It seems like this could hang the test.

Fixed in the latest patch by exiting the loop after N number of iterations.

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 443038 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebKit/PreferenceChanges.mm:191
>>> +        TestWebKitAPI::Util::spinRunLoop();
>> 
>> This will potentially slow down the process. Is it possible to wait until a sync has happened (perhaps using an event or completion handler)?
> 
> That is a good point. I think we for example could post a notification in the WebContent process when a preference is set, which TestWebKitAPI could then subscribe to. There is some ambiguity to this approach, however, since we're not using WebKit API to set the preference, but using NSUserDefaults directly. This means it is not trivial to match the preference change made by TestWebKitAPI with the corresponding notification, since the preference change could also have come from elsewhere. Due to this ambiguity, I did not go down this road, but added a sleep interval to the loop in order to avoid using too much CPU. Would you be OK with that approach?

Is there no notification sent when NSUserDefaults changes a value? I believe that a process can subscribe to notifications so that when a user changes a preference the app can react. It might be that we receive notifications for unrelated preference changes, so we would want to see what preference changed and ignore unrelated ones.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #9)
> Comment on attachment 443038 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443038&action=review
> 
> >>> Tools/TestWebKitAPI/Tests/WebKit/PreferenceChanges.mm:191
> >>> +        TestWebKitAPI::Util::spinRunLoop();
> >> 
> >> This will potentially slow down the process. Is it possible to wait until a sync has happened (perhaps using an event or completion handler)?
> > 
> > That is a good point. I think we for example could post a notification in the WebContent process when a preference is set, which TestWebKitAPI could then subscribe to. There is some ambiguity to this approach, however, since we're not using WebKit API to set the preference, but using NSUserDefaults directly. This means it is not trivial to match the preference change made by TestWebKitAPI with the corresponding notification, since the preference change could also have come from elsewhere. Due to this ambiguity, I did not go down this road, but added a sleep interval to the loop in order to avoid using too much CPU. Would you be OK with that approach?
> 
> Is there no notification sent when NSUserDefaults changes a value? I believe
> that a process can subscribe to notifications so that when a user changes a
> preference the app can react. It might be that we receive notifications for
> unrelated preference changes, so we would want to see what preference
> changed and ignore unrelated ones.

I believe it is possible to use KVO to listen to changes for a specific preference key. Since this requires sandbox access to the preference daemon, which is blocked in the WebContent process, it might not be applicable in this case. Should we create a custom notification that we post from WP when a preference change message is received? TestWebKitAPI could then listen to this notification, and we could avoid the loop in the test.

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 443038 [details]
Patch

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

>>>>> Tools/TestWebKitAPI/Tests/WebKit/PreferenceChanges.mm:191
>>>>> +        TestWebKitAPI::Util::spinRunLoop();
>>>> 
>>>> This will potentially slow down the process. Is it possible to wait until a sync has happened (perhaps using an event or completion handler)?
>>> 
>>> That is a good point. I think we for example could post a notification in the WebContent process when a preference is set, which TestWebKitAPI could then subscribe to. There is some ambiguity to this approach, however, since we're not using WebKit API to set the preference, but using NSUserDefaults directly. This means it is not trivial to match the preference change made by TestWebKitAPI with the corresponding notification, since the preference change could also have come from elsewhere. Due to this ambiguity, I did not go down this road, but added a sleep interval to the loop in order to avoid using too much CPU. Would you be OK with that approach?
>> 
>> Is there no notification sent when NSUserDefaults changes a value? I believe that a process can subscribe to notifications so that when a user changes a preference the app can react. It might be that we receive notifications for unrelated preference changes, so we would want to see what preference changed and ignore unrelated ones.
> 
> I believe it is possible to use KVO to listen to changes for a specific preference key. Since this requires sandbox access to the preference daemon, which is blocked in the WebContent process, it might not be applicable in this case. Should we create a custom notification that we post from WP when a preference change message is received? TestWebKitAPI could then listen to this notification, and we could avoid the loop in the test.
> 
> Thanks for reviewing!

Ah! That's a great reason to not use KVO. Thanks for clarifying.

[Brent Fulgham]

Comment on attachment 443103 [details]
Patch

r=me

[Per Arne Vollan]

Comment on attachment 443103 [details]
Patch

Thanks for reviewing!

[EWS]

Committed r285184 (243813@main): <https://commits.webkit.org/243813@main>

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