Bug 194954 - Regression(PSON) Unable to preview password-protected documents on iCloud.com
Summary: Regression(PSON) Unable to preview password-protected documents on iCloud.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-22 11:36 PST by Chris Dumez
Modified: 2019-02-23 08:13 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.96 KB, patch)
2019-02-22 11:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.61 KB, patch)
2019-02-22 12:27 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.73 KB, patch)
2019-02-22 12:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.70 KB, patch)
2019-02-22 12:48 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.58 KB, patch)
2019-02-22 12:55 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-02-22 11:36:58 PST
Unable to preview password-protected documents on iCloud.com when PSON is enabled.
Comment 1 Chris Dumez 2019-02-22 11:37:13 PST
<rdar://problem/48127957>
Comment 2 Chris Dumez 2019-02-22 11:38:43 PST
Created attachment 362736 [details]
Patch
Comment 3 Chris Dumez 2019-02-22 11:57:19 PST
Comment on attachment 362736 [details]
Patch

Trying to write an API test...
Comment 4 Chris Dumez 2019-02-22 12:27:14 PST
Created attachment 362745 [details]
Patch
Comment 5 EWS Watchlist 2019-02-22 12:30:23 PST
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.
Comment 6 Alex Christensen 2019-02-22 12:37:38 PST
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.
Comment 7 Chris Dumez 2019-02-22 12:40:31 PST
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.
Comment 8 Chris Dumez 2019-02-22 12:44:06 PST
Created attachment 362750 [details]
Patch
Comment 9 EWS Watchlist 2019-02-22 12:46:36 PST
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.
Comment 10 Chris Dumez 2019-02-22 12:46:54 PST
(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.
Comment 11 Chris Dumez 2019-02-22 12:48:51 PST
Created attachment 362753 [details]
Patch
Comment 12 EWS Watchlist 2019-02-22 12:50:39 PST
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.
Comment 13 Chris Dumez 2019-02-22 12:55:05 PST
Created attachment 362754 [details]
Patch
Comment 14 EWS Watchlist 2019-02-22 12:57:47 PST
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.
Comment 15 Chris Dumez 2019-02-22 13:17:50 PST
Comment on attachment 362754 [details]
Patch

Clearing flags on attachment: 362754

Committed r241963: <https://trac.webkit.org/changeset/241963>
Comment 16 Chris Dumez 2019-02-22 13:17:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Alexey Proskuryakov 2019-02-23 00:50:34 PST
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.
Comment 18 Chris Dumez 2019-02-23 08:13:33 PST
(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.