Regression(AsyncPolicyDelegates): Box.app login Window is blank

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Box.app login Window is blank since we make the policy delegates fully asynchronous.

[Chris Dumez]

<rdar://problem/40307871>

[Chris Dumez]

Created attachment 340874 [details]
Patch

[Alex Christensen]

Comment on attachment 340874 [details]
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:20
> +        instead of retaining it. If the policy listener object gets destroyed because getting

*before

[Chris Dumez]

Created attachment 340876 [details]
Patch

[Geoffrey Garen]

EWS is crashing on every test. Is that from this patch?

[EWS Watchlist]

Comment on attachment 340876 [details]
Patch

Attachment 340876 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7756320

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340885 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

(In reply to Geoffrey Garen from comment #5)
> EWS is crashing on every test. Is that from this patch?

Possibly. I am investigating :)

[Chris Dumez]

Created attachment 340887 [details]
Patch

[Chris Dumez]

Created attachment 340888 [details]
Patch

[Geoffrey Garen]

Probably worth saying that we're doing this for backwards compatibility. It's not necessarily the behavior I would choose from first principles.

[Chris Dumez]

Comment on attachment 340888 [details]
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:21
> +        resolved, we now use "Use" policy action in its dealloc function to maintain previous

The change log already says so here.

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:2435
> +    // If the app did not respond before the listener is destroyed, then we let the load

Would you like me to state it as well in this comment?

[EWS Watchlist]

Comment on attachment 340888 [details]
Patch

Attachment 340888 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7757463

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340901 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Crashes are related. Looks like it is crashing when trying to null-check the weak data member:
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000080080020
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

VM Regions Near 0x80080020:
--> 
    __TEXT                 000000010918e000-00000001091e5000 [  348K] r-x/rwx SM=COW  /Volumes/VOLUME/*

Application Specific Information:
CRASHING TEST: accessibility/mac/value-change/value-change-user-info-contenteditable.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib               	0x00007fffd7b46c2b objc_loadWeakRetained + 176
1   libobjc.A.dylib               	0x00007fffd7b48acb objc_loadWeak + 15
2   com.apple.WebKitLegacy        	0x000000010f5473d5 WebFrameLoaderClient::cancelPolicyCheck() + 21 (WebFrameLoaderClient.mm:917)
3   com.apple.WebCore             	0x000000010ce6533d WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL, WebCore::NavigationPolicyCheck, WTF::CompletionHandler<void ()>&&) + 1117 (DumbPtrTraits.h:41)
4   com.apple.WebCore             	0x000000010ce62a87 WebCore::FrameLoader::load(WebCore::DocumentLoader*) + 327 (memory:2733)
5   com.apple.WebCore             	0x000000010ce64e2b WebCore::FrameLoader::load(WebCore::FrameLoadRequest&&) + 843 (SetForScope.h:61)
6   com.apple.WebKitLegacy        	0x000000010f542d93 -[WebFrame loadRequest:] + 371 (WebFrame.mm:2489)
7   DumpRenderTree                	0x00000001091a0c11 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2716 (DumpRenderTree.mm:1999)
8   DumpRenderTree                	0x000000010919ff86 dumpRenderTree(int, char const**) + 3115 (DumpRenderTree.mm:1163)
9   DumpRenderTree                	0x00000001091a169a DumpRenderTreeMain(int, char const**) + 1476 (DumpRenderTree.mm:1383)
10  libdyld.dylib                 	0x00007fffd842e235 start + 1

[EWS Watchlist]

Comment on attachment 340888 [details]
Patch

Attachment 340888 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7757379

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340909 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 340888 [details]
Patch

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

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h:263
> +    __weak WebFramePolicyListener *m_policyListener;

Looks like I may need to use WeakObjCPtr<> class since this is C++.

[Chris Dumez]

Created attachment 340916 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakObjCPtr.h:36:  objc_loadWeakRetained is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:37:  objc_initWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:38:  objc_destroyWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:39:  objc_copyWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:40:  objc_moveWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 44 files


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

[Chris Dumez]

Created attachment 340920 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakObjCPtr.h:36:  objc_loadWeakRetained is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:37:  objc_initWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:38:  objc_destroyWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:39:  objc_copyWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:40:  objc_moveWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 45 files


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

[Chris Dumez]

Created attachment 340921 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakObjCPtr.h:36:  objc_loadWeakRetained is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:37:  objc_initWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:38:  objc_destroyWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:39:  objc_copyWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/WeakObjCPtr.h:40:  objc_moveWeak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 47 files


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

[Chris Dumez]

Comment on attachment 340921 [details]
Patch

Setting review flag again because I had to move WeakObjCPtr to wtf/ and use it instead of __weak, to fix crashes on EWS.

[Geoffrey Garen]

Comment on attachment 340921 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 340921 [details]
Patch

Clearing flags on attachment: 340921

Committed r232082: <https://trac.webkit.org/changeset/232082>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Comment on attachment 340921 [details]
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:22
> +        resolved, we now use "Use" policy action in its dealloc function to maintain previous
> +        behavior.

Was this really the previous behavior?

[Chris Dumez]

(In reply to mitz from comment #29)
> Comment on attachment 340921 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340921&action=review
> 
> > Source/WebKitLegacy/mac/ChangeLog:22
> > +        resolved, we now use "Use" policy action in its dealloc function to maintain previous
> > +        behavior.
> 
> Was this really the previous behavior?

It was not voluntary but we basically would not wait for the client's response when the client did not respond synchronously. We would start receiving the network data and even finish the load if the client did not respond in time. So yes, if the client did not answer at all, the load would proceed and would be the same as policy "Use".

See Bug 182720.

(In reply to Chris Dumez from comment #30)
> (In reply to mitz from comment #29)
> > Comment on attachment 340921 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=340921&action=review
> > 
> > > Source/WebKitLegacy/mac/ChangeLog:22
> > > +        resolved, we now use "Use" policy action in its dealloc function to maintain previous
> > > +        behavior.
> > 
> > Was this really the previous behavior?
> 
> It was not voluntary but we basically would not wait for the client's
> response when the client did not respond synchronously. We would start
> receiving the network data and even finish the load if the client did not
> respond in time. So yes, if the client did not answer at all, the load would
> proceed and would be the same as policy "Use".
> 
> See Bug 182720.

The claim you are making here is even stronger than what I asked: you are saying that if the client did not make the policy decision immediately, WebKit would proceed with the load. It’s obvious to me that that isn’t the behavior of the API in any shipping version of macOS, as such behavior would break Safari’s “do you want to submit this form again” sheets, to name one example. Perhaps a recent regression on TOT caused confusion over what the shipping behavior has always been.

[Chris Dumez]

Comment on attachment 340921 [details]
Patch

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

>>>> Source/WebKitLegacy/mac/ChangeLog:22
>>>> +        behavior.
>>> 
>>> Was this really the previous behavior?
>> 
>> It was not voluntary but we basically would not wait for the client's response when the client did not respond synchronously. We would start receiving the network data and even finish the load if the client did not respond in time. So yes, if the client did not answer at all, the load would proceed and would be the same as policy "Use".
>> 
>> See Bug 182720.
> 
> The claim you are making here is even stronger than what I asked: you are saying that if the client did not make the policy decision immediately, WebKit would proceed with the load. It’s obvious to me that that isn’t the behavior of the API in any shipping version of macOS, as such behavior would break Safari’s “do you want to submit this form again” sheets, to name one example. Perhaps a recent regression on TOT caused confusion over what the shipping behavior has always been.

You are talking about decidePolicyForNavigationAction and I am talking about decidePolicyForNavigationResponse. And this is a very good point, looking at this patch, it looks like my "Default to policy USE" behavior impacts both policy decisions for navigation actions and responses :/ This was unintentional. The fallback to policy USE should only be for decidePolicyForNavigationResponse, not decidePolicyForNavigationAction. I will look into fixing this.