Bug 156117 – LoadAlternateHTMLString should be refined

• Component: WebKit2
• Status: NEW

[Jiewen Tan]

Created attachment 275428 [details]
Test Case

Current SPI: _LoadAlternateHTMLString will prohibit any provisional load failures from being sent to clients while handling provisional load failures. This is too heavy. For example, the current load will fail to cancel another ongoing load. That might prevent clients' page load state being handled properly.

There is the discussion from Bug 155995 to detail explain the problem:
> Comment on attachment 275254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275254&action=review
> 
> >>>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1008
> >>>> +    if (m_isClosed || m_isDealingFailingProvisionalLoad)
> >>> 
> >>> Wouldn't any new load started in response to a provisional load failure cause this problem? For instance, if the client called loadRequest: instead of _loadAlternateHTMLString:baseURL:forUnreachableURL:, would you see the same assertion? If so, seems like we need a fix that won't involve adding checks like this to every loading entry point in the UI process. I'm ok with this for now, since you are handling what Safari does, but we might need to come up with a better long-term solution.
> >> 
> >> I don't think other APIs have the same problem. It should be only specific to loadAlternateHTMLString as the way we tell if we are in the process of responding to a provisional load failure in WebProcess is to check whether FrameLoader ::m_provisionalLoadErrorBeingHandledURL is empty. This member is only set/clear at WebPage::loadAlternateHTMLString.
> > 
> > No, that's not true. m_provisionalLoadErrorBeingHandledURL is set in FrameLoader whenever there's a provisional load failure. It's also set in WebPage::loadAlternateHTMLString(), but that's just to account for the fact that WebKit2 clients do not load alternate HTML synchronously.
> > 
> > My question was whether the assertion failure you saw would also occur in the presence of two sequential provisionally failing loads when the client responded to the second one with something other than _loadAlternateHTMLString:...
> 
> I think the situation you mentioned is fine, but what will cause problems is
> replacing the first one with some other load APIs.
> 
> Once WebProcess enter the state of loadAlternateHTMLString, it will lose the
> ability to send DidFailProvisionalLoad error through
> checkLoadCompleteForThisFrame to the clients, and therefore any back to back
> load before loadAlternateHTMLString will cause the assertion to hit.
> 
> The way how loadAlternateHTMLString lock things down is too heavy. I don't
> have any idea to fix this for long term so far.

It isn't urgent since we don't know if any clients actually do this, but filing a bug and writing some test cases is a good place to start. Thanks!
[reply] [-] Comment 21

Also see FIXME on FrameLoader::checkLoadCompleteForThisFrame + 6:
// FIXME: Prohibiting any provisional load failures from being sent to clients
// while handling provisional load failures is too heavy. For example, the current
// load will fail to cancel another ongoing load. That might prevent clients' page
// load state being handled properly.

A test case to illustrate the improper handle of page load state and a corresponding crash log is uploaded. BTW, the actual assertion that is triggered is in:
PageLoadState::didStartProvisionalLoad + 3: ASSERT(m_uncommittedState.provisionalURL.isEmpty());

[Jiewen Tan]

Created attachment 275429 [details]
Crash Log

[Radar WebKit Bug Importer]

<rdar://problem/25500444>