Bug 194954

Summary:

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

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebKit2

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, ap, beidson, ews-watchlist, ggaren, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Chris Dumez

Reported 2019-02-22 11:36:58 PST

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

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2019-02-22 11:37:13 PST

<rdar://problem/48127957>

Chris Dumez

Comment 2 2019-02-22 11:38:43 PST

Created attachment 362736 [details] Patch

Chris Dumez

Comment 3 2019-02-22 11:57:19 PST

Comment on attachment 362736 [details] Patch Trying to write an API test...

Chris Dumez

Comment 4 2019-02-22 12:27:14 PST

Created attachment 362745 [details] Patch

EWS Watchlist

Comment 5 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.

Alex Christensen

Comment 6 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.

Chris Dumez

Comment 7 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.

Chris Dumez

Comment 8 2019-02-22 12:44:06 PST

Created attachment 362750 [details] Patch

EWS Watchlist

Comment 9 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.

Chris Dumez

Comment 10 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.

Chris Dumez

Comment 11 2019-02-22 12:48:51 PST

Created attachment 362753 [details] Patch

EWS Watchlist

Comment 12 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.

Chris Dumez

Comment 13 2019-02-22 12:55:05 PST

Created attachment 362754 [details] Patch

EWS Watchlist

Comment 14 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.

Chris Dumez

Comment 15 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>

Chris Dumez

Comment 16 2019-02-22 13:17:51 PST

All reviewed patches have been landed. Closing bug.

Alexey Proskuryakov

Comment 17 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.

Chris Dumez

Comment 18 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.

Note You need to log in before you can comment on or make changes to this bug.