[EFL][WK2] Context clients should unregister themselves when destroyed

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

We get crashes after a Ewk_Context is destroyed because the corresponding client classes get destroyed but they don't unregister their callbacks. As a consequence, the client callbacks sometimes get called after the client objects have been destroyed and it causes crashes.

We should make sure all our client classes unregister their callbacks in their class destructor.

[Chris Dumez]

Created attachment 177726 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 177726 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.cpp:137
> +    m_context->initializeHistoryClient(0);

hmm maybe we should rename to not be called initialize then, and use setHistoryClient

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 177726 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177726&action=review
> 
> > Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.cpp:137
> > +    m_context->initializeHistoryClient(0);
> 
> hmm maybe we should rename to not be called initialize then, and use setHistoryClient

Well, the corresponding C API is WKContextSetHistoryClient() so I tend to agree with you.

However, I believe this should be done in a separate patch and we will need to CC Andersca.

For now, what matters is that we get the crashes fixed.

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 177726 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177726&action=review
> > 
> > > Source/WebKit2/UIProcess/efl/ContextHistoryClientEfl.cpp:137
> > > +    m_context->initializeHistoryClient(0);
> > 
> > hmm maybe we should rename to not be called initialize then, and use setHistoryClient
> 
> Well, the corresponding C API is WKContextSetHistoryClient() so I tend to agree with you.
> 
> However, I believe this should be done in a separate patch and we will need to CC Andersca.
> 
> For now, what matters is that we get the crashes fixed.

Kenneth, do you mind if we land this patch like it is for now and propose the renaming afterwards? Or do you want to do the renaming beforehand?

[Kenneth Rohde Christiansen]

Comment on attachment 177726 [details]
Patch

no

[WebKit Review Bot]

Comment on attachment 177726 [details]
Patch

Clearing flags on attachment: 177726

Committed r136833: <http://trac.webkit.org/changeset/136833>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.