Summary: | Crash under WebProcessProxy::didBecomeUnresponsive() | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, ggaren, rniwa, webkit-bug-importer, youennf | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=197883 | ||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2019-08-01 09:26:07 PDT
Created attachment 375313 [details]
Patch
Created attachment 375316 [details]
Patch
Created attachment 375317 [details]
Patch
Comment on attachment 375317 [details]
Patch
r=me
I wonder why we don't just hold a RefPtr to our client -- or why our client doesn't clear itself when destroyed?
Comment on attachment 375317 [details]
Patch
CSSFontFaceSet does something a little cleaner than this.
(In reply to Geoffrey Garen from comment #5) > Comment on attachment 375317 [details] > Patch > > r=me > > I wonder why we don't just hold a RefPtr to our client You mean have the ResponsivenessTimer::Client subclass RefCounted<> instead of WebProcessProxy? > -- or why our client > doesn't clear itself when destroyed? The issue is that the client is the WebProcessProxy and that it has 2 data members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is no point in having the client unregister itself from the Timer at the moment because the Timer owns the Timer. Once the client is dead, so is the Timer. (In reply to Chris Dumez from comment #7) > (In reply to Geoffrey Garen from comment #5) > > Comment on attachment 375317 [details] > > Patch > > > > r=me > > > > I wonder why we don't just hold a RefPtr to our client > > You mean have the ResponsivenessTimer::Client subclass RefCounted<> instead > of WebProcessProxy? > > > -- or why our client > > doesn't clear itself when destroyed? > > The issue is that the client is the WebProcessProxy and that it has 2 data > members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is > no point in having the client unregister itself from the Timer at the moment > because the Timer owns the Timer. Once the client is dead, so is the Timer. The *Client* owns the Timer. Created attachment 375330 [details]
Patch
Created attachment 375331 [details]
Patch
> > -- or why our client
> > doesn't clear itself when destroyed?
>
> The issue is that the client is the WebProcessProxy and that it has 2 data
> members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is
> no point in having the client unregister itself from the Timer at the moment
> because the [ client ] owns the Timer. Once the client is dead, so is the Timer.
I see: Our theory of this crash is that the client is live at the time of timer fire, but dies afterward?
I guess that's possible. WebProcessProxy::didBecomeUnresponsive() already does "auto protectedThis = makeRef(*this);", but WebProcessProxy::didBecomeResponsive(), WebProcessProxy::willChangeIsResponsive(), and WebProcessProxy::didChangeIsResponsive() do not.
Do we also need to cover ResponsivenessTimer::stop()?
(In reply to Geoffrey Garen from comment #11) > > > -- or why our client > > > doesn't clear itself when destroyed? > > > > The issue is that the client is the WebProcessProxy and that it has 2 data > > members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is > > no point in having the client unregister itself from the Timer at the moment > > because the [ client ] owns the Timer. Once the client is dead, so is the Timer. > > I see: Our theory of this crash is that the client is live at the time of > timer fire, but dies afterward? Yes, this is my latest theory since my protector inside WebProcessProxy::didBecomeUnresponsive() did not fix this crash. > > I guess that's possible. WebProcessProxy::didBecomeUnresponsive() already > does "auto protectedThis = makeRef(*this);", but > WebProcessProxy::didBecomeResponsive(), > WebProcessProxy::willChangeIsResponsive(), and > WebProcessProxy::didChangeIsResponsive() do not. Right. > Do we also need to cover ResponsivenessTimer::stop()? Same pattern so it seems better to cover it too. I'll update the patch. Created attachment 375332 [details]
Patch
Created attachment 375333 [details]
Patch
Created attachment 375334 [details]
Patch
Comment on attachment 375334 [details] Patch Clearing flags on attachment: 375334 Committed r248121: <https://trac.webkit.org/changeset/248121> All reviewed patches have been landed. Closing bug. |