ASSERT_NOT_REACHED() under WebPageProxy::suspendCurrentPageIfPossible()

[Chris Dumez]

ASSERT_NOT_REACHED() under WebPageProxy::suspendCurrentPageIfPossible() when doing a client-side redirect cross-site in an ephemeral session:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010eb251fe WTFCrash + 14 (Assertions.cpp:305)
1   com.apple.WebKit              	0x0000000119e9869b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebKit              	0x000000011aba3aa5 WebKit::WebPageProxy::suspendCurrentPageIfPossible(API::Navigation&, WTF::Optional<WTF::ObjectIdentifier<WebCore::FrameIdentifierType> >, WebKit::ProcessSwapRequestedByClient, WebKit::ShouldDelayClosingUntilEnteringAcceleratedCompositingMode) + 1909 (WebPageProxy.cpp:814)
3   com.apple.WebKit              	0x000000011abb827a WebKit::WebPageProxy::commitProvisionalPage(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&) + 1338 (WebPageProxy.cpp:3200)
4   com.apple.WebKit              	0x000000011aa2fc79 WebKit::ProvisionalPageProxy::didCommitLoadForFrame(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&) + 953
5   com.apple.WebKit              	0x000000011aa72eaa void IPC::callMemberFunctionImpl<WebKit::ProvisionalPageProxy, void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String, bool, unsigned int, WebCore::CertificateInfo, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData>, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul>(WebKit::ProvisionalPageProxy*, void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String, bool, unsigned int, WebCore::CertificateInfo, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul>) + 538 (HandleMessage.h:42)
6   com.apple.WebKit              	0x000000011aa6a480 void IPC::callMemberFunction<WebKit::ProvisionalPageProxy, void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String, bool, unsigned int, WebCore::CertificateInfo, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul> >(std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String, bool, unsigned int, WebCore::CertificateInfo, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData>&&, WebKit::ProvisionalPageProxy*, void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&)) + 112 (HandleMessage.h:48)
7   com.apple.WebKit              	0x000000011aa32b9c void IPC::handleMessage<Messages::WebPageProxy::DidCommitLoadForFrame, WebKit::ProvisionalPageProxy, void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&)>(IPC::Decoder&, WebKit::ProvisionalPageProxy*, void (WebKit::ProvisionalPageProxy::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, unsigned long long, WTF::String const&, bool, unsigned int, WebCore::CertificateInfo const&, bool, bool, WTF::Optional<WebCore::HasInsecureContent>, WebKit::UserData const&)) + 268 (HandleMessage.h:121)
8   com.apple.WebKit              	0x000000011aa317ee WebKit::ProvisionalPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 2654 (ProvisionalPageProxy.cpp:477)
9   com.apple.WebKit              	0x0000000119fa372b IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 315 (MessageReceiverMap.cpp:124)
10  com.apple.WebKit              	0x000000011a9d99ee WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 46 (AuxiliaryProcessProxy.cpp:196)

[Chris Dumez]

<rdar://problem/59464606>

[Chris Dumez]

Created attachment 391001 [details]
Patch

[John Wilander]

Comment on attachment 391001 [details]
Patch

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

See the one comment.

> Source/WebCore/loader/HistoryController.cpp:554
> +    bool needsPrivacy = page->usesEphemeralSession();

Privacy is such a broad term. I'd prefer to stick with ephemeral as the term because that's the only privacy promise it makes. Would isEphemeral work? Or just usesEphemeralSession?

[Chris Dumez]

Comment on attachment 391001 [details]
Patch

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

>> Source/WebCore/loader/HistoryController.cpp:554
>> +    bool needsPrivacy = page->usesEphemeralSession();
> 
> Privacy is such a broad term. I'd prefer to stick with ephemeral as the term because that's the only privacy promise it makes. Would isEphemeral work? Or just usesEphemeralSession?

I chose this to be consistent with the rest of this class (see other uses of usesEphemeralSession in this class). What do you want me to do, rename all of them?

[John Wilander]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 391001 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391001&action=review
> 
> >> Source/WebCore/loader/HistoryController.cpp:554
> >> +    bool needsPrivacy = page->usesEphemeralSession();
> > 
> > Privacy is such a broad term. I'd prefer to stick with ephemeral as the term because that's the only privacy promise it makes. Would isEphemeral work? Or just usesEphemeralSession?
> 
> I chose this to be consistent with the rest of this class (see other uses of
> usesEphemeralSession in this class). What do you want me to do, rename all
> of them?

That's unfortunate. Then either way is fine with me, i.e. fix this one instance now or land it consistent with the rest.

[Chris Dumez]

Created attachment 391046 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391046 [details]
Patch

Clearing flags on attachment: 391046

Committed r256831: <https://trac.webkit.org/changeset/256831>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.