Reloading a view in its processTerminationHandler does not work reliably when using related views

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Reloading a view in its processTerminationHandler does not work reliable when using related views.

[Chris Dumez]

Created attachment 410702 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70019867>

[Geoffrey Garen]

Comment on attachment 410702 [details]
Patch

r=me

[Chris Dumez]

Created attachment 410703 [details]
Patch

[EWS]

Committed r268097: <https://trac.webkit.org/changeset/268097>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410703 [details].

[Michael Catanzaro]

This introduced a regression in Epiphany. Previously, our web-process-terminated signal handler checked the URI of the web view to decide which page to reload. The Reload button in our crash page is hooked up to some JS that sets window.location to this URI. But since this commit, the web view changes its URI to an empty string before it emits web-process-terminated. So now when users click the Reload button, nothing happens.

It's very easy to work around in Epiphany: we could simply remember the last non-empty URL and use that when loading the crash page. That's a bit messy, but it seems better than going back to synchronous loading that doesn't work properly for related views. But I wouldn't be surprised if other WebKit clients might be broken in the same way.

[Michael Catanzaro]

Hm, even vanilla reload() cannot work anymore, because it would only reload an empty address and wind up with about:blank. So although we *can* work around this in the client, I'm not sure we should, because it seems like all clients would be affected. (Are you sure this didn't break Safari?)

[Chris Dumez]

(In reply to Michael Catanzaro from comment #7)
> Hm, even vanilla reload() cannot work anymore, because it would only reload
> an empty address and wind up with about:blank. So although we *can* work
> around this in the client, I'm not sure we should, because it seems like all
> clients would be affected. (Are you sure this didn't break Safari?)

It did break Safari iirc and we fixed Safari.

[Michael Catanzaro]

OK, my proposed Epiphany fix is https://gitlab.gnome.org/GNOME/epiphany/-/merge_requests/913

[Chris Dumez]

(In reply to Michael Catanzaro from comment #9)
> OK, my proposed Epiphany fix is
> https://gitlab.gnome.org/GNOME/epiphany/-/merge_requests/913

BTW, I would expect reload() to work. As a matter of fact, this is what WebKit's default crash handler does (when the client does not set a custom crash handler).

[Michael Catanzaro]

That doesn't work on its own because before we can reload(), we have to do webkit_web_view_load_alternate_html(), and then whatever URI we pass to that is what will get reloaded. So we need to track the previous nonempty URI regardless, or we'll just wind up reloading an empty string.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #11)
> That doesn't work on its own because before we can reload(), we have to do
> webkit_web_view_load_alternate_html(), and then whatever URI we pass to that
> is what will get reloaded. So we need to track the previous nonempty URI
> regardless, or we'll just wind up reloading an empty string.

I see. I agree this is risky. I will see if I can find a better way to fix this.

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > That doesn't work on its own because before we can reload(), we have to do
> > webkit_web_view_load_alternate_html(), and then whatever URI we pass to that
> > is what will get reloaded. So we need to track the previous nonempty URI
> > regardless, or we'll just wind up reloading an empty string.
> 
> I see. I agree this is risky. I will see if I can find a better way to fix
> this.

Fixing via Bug 222011.

[Michael Catanzaro]

Ah cool, thanks.