Bug 194954 – Regression(PSON) Unable to preview password-protected documents on iCloud.com

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Unable to preview password-protected documents on iCloud.com when PSON is enabled.

[Chris Dumez]

<rdar://problem/48127957>

[Chris Dumez]

Created attachment 362736 [details]
Patch

[Chris Dumez]

Comment on attachment 362736 [details]
Patch

Trying to write an API test...

[Chris Dumez]

Created attachment 362745 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:180:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:5345:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 2 in 7 files


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

[Alex Christensen]

Comment on attachment 362745 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1167
> +    pageClient().requestPasswordForQuickLookDocument(fileName, [protectedThis = makeRef(*this), process = WTFMove(process)](const String& password) {

I'm not sure you would need to protect this here.

[Chris Dumez]

Comment on attachment 362745 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1167
>> +    pageClient().requestPasswordForQuickLookDocument(fileName, [protectedThis = makeRef(*this), process = WTFMove(process)](const String& password) {
> 
> I'm not sure you would need to protect this here.

I think I need to. I understand that a page normally keeps its process alive. However, with PSON, the process associated with a page may change. E.g. You start a provisional load, which requests a password, then you start another cross-site provisional load which cancels the first one.
By the time the client calls the completion handler, the process may be dead.

[Chris Dumez]

Created attachment 362750 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:73:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:183:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:5348:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 3 in 7 files


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

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 362745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362745&action=review
> 
> >> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1167
> >> +    pageClient().requestPasswordForQuickLookDocument(fileName, [protectedThis = makeRef(*this), process = WTFMove(process)](const String& password) {
> > 
> > I'm not sure you would need to protect this here.
> 
> I think I need to. I understand that a page normally keeps its process
> alive. However, with PSON, the process associated with a page may change.
> E.g. You start a provisional load, which requests a password, then you start
> another cross-site provisional load which cancels the first one.
> By the time the client calls the completion handler, the process may be dead.

Oh, I misread your comment. You're talking about protecting *this*. I think you're right, assuming I capture the pageID.

[Chris Dumez]

Created attachment 362753 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:73:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:183:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:5348:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 3 in 7 files


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

[Chris Dumez]

Created attachment 362754 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:73:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:183:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:5348:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 3 in 7 files


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

[Chris Dumez]

Comment on attachment 362754 [details]
Patch

Clearing flags on attachment: 362754

Committed r241963: <https://trac.webkit.org/changeset/241963>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 362754 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:73
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000

Why? We don’t support iOS 10.

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #17)
> Comment on attachment 362754 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362754&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:73
> > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000
> 
> Why? We don’t support iOS 10.

I copied it from an existing quicklook password test.