Bug 217407 - Reloading a view in its processTerminationHandler does not work reliably when using related views
Summary: Reloading a view in its processTerminationHandler does not work reliably when...
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: 222011 222809
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-06 15:13 PDT by Chris Dumez
Modified: 2021-03-05 10:38 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.03 KB, patch)
2020-10-06 15:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2020-10-06 15:32 PDT, 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 2020-10-06 15:13:09 PDT
Reloading a view in its processTerminationHandler does not work reliable when using related views.
Comment 1 Chris Dumez 2020-10-06 15:20:40 PDT
Created attachment 410702 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-10-06 15:23:27 PDT
<rdar://problem/70019867>
Comment 3 Geoffrey Garen 2020-10-06 15:29:05 PDT
Comment on attachment 410702 [details]
Patch

r=me
Comment 4 Chris Dumez 2020-10-06 15:32:11 PDT
Created attachment 410703 [details]
Patch
Comment 5 EWS 2020-10-06 17:21:07 PDT
Committed r268097: <https://trac.webkit.org/changeset/268097>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410703 [details].
Comment 6 Michael Catanzaro 2021-02-10 08:38:30 PST
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.
Comment 7 Michael Catanzaro 2021-02-16 08:00:49 PST
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?)
Comment 8 Chris Dumez 2021-02-16 08:27:21 PST
(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.
Comment 9 Michael Catanzaro 2021-02-16 09:06:15 PST
OK, my proposed Epiphany fix is https://gitlab.gnome.org/GNOME/epiphany/-/merge_requests/913
Comment 10 Chris Dumez 2021-02-16 09:07:53 PST
(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).
Comment 11 Michael Catanzaro 2021-02-16 09:14:19 PST
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.
Comment 12 Chris Dumez 2021-02-16 17:14:46 PST
(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.
Comment 13 Chris Dumez 2021-02-16 17:37:27 PST
(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.
Comment 14 Michael Catanzaro 2021-02-17 06:35:52 PST
Ah cool, thanks.