Bug 181472 – [WPE] TestWebKitFindController asserts

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Ms2ger (he/him; ⌚ UTC+1/+2)]

In the _WebKitWebViewPrivate destructor, we set this->view to nullptr. We then automatically destruct this->findController, and end up calling webkitFindControllerDispose, which calls webkitWebViewGetPage()... But that asserts webView->priv->view is non-null.

[Michael Catanzaro]

Carlos Garcia, do you remember why WKWPE::View has to be destroyed before webkitWebViewBackendUnref? That's not helping things.

[Michael Catanzaro]

Created attachment 330935 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 330935 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:202
> +        findController = nullptr;

I'm not sure this is the right fix. Objects keeping a pointer to the view and not taking a reference should use g_object_add_weak_pointer() like WebKitDownload does.

[Michael Catanzaro]

Good idea, that's safer.

(At least, it's safer as long as we do it right... I dislike that GObject weak pointers cause random un-debuggable memory corruption when misused.)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Good idea, that's safer.

No wait, it's not a great idea. WebKitWebView holds a ref on the WebKitFindController, so there's no reason for WebKitFindController to need a weak pointer. WebKitWebView should be valid for its entire lifetime. It's simpler to just fix the order of destruction.

The objects are ordered properly in _WebKitWebViewPrivate to ensure that the WebKitFindController is destroyed before the WKWPE::View, but that is broken by manually destroying the WKWPE::View before calling webkitWebViewBackendUnref.

WebKitDownload is a very different case, because WebKitDownload is not owned by the WebKitWebView and indeed regularly outlives its associated WebKitWebView.

[Michael Catanzaro]

Created attachment 331068 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> Created attachment 331068 [details]
> Patch

Alternative approach using a GRefPtr. Not sure where the best place to declare the overloads is.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Michael Catanzaro from comment #5)
> > Good idea, that's safer.
> 
> No wait, it's not a great idea. WebKitWebView holds a ref on the
> WebKitFindController, so there's no reason for WebKitFindController to need
> a weak pointer. WebKitWebView should be valid for its entire lifetime.

You are right.

> It's
> simpler to just fix the order of destruction.
> 
> The objects are ordered properly in _WebKitWebViewPrivate to ensure that the
> WebKitFindController is destroyed before the WKWPE::View, but that is broken
> by manually destroying the WKWPE::View before calling
> webkitWebViewBackendUnref.
> 
> WebKitDownload is a very different case, because WebKitDownload is not owned
> by the WebKitWebView and indeed regularly outlives its associated
> WebKitWebView.

Indeed, I agree.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #1)
> Carlos Garcia, do you remember why WKWPE::View has to be destroyed before
> webkitWebViewBackendUnref? That's not helping things.

Because the wpe backend is only owned by WebKitWebViewBackend, and WKView maintains a pointer to the wpe backend. We don't want to handle the case of wpe backend being nullptr in WKView or adding a way to notify WKView when wpe backend has been destroyed, so this way we ensure the backend is alive for the entire life of WKView.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> (In reply to Michael Catanzaro from comment #7)
> > Created attachment 331068 [details]
> > Patch
> 
> Alternative approach using a GRefPtr. Not sure where the best place to
> declare the overloads is.

What's the difference? GRefPtr will not ensure the view is destroyed before the backend, no? is it defined the order in which things happen in destructors?

[Carlos Garcia Campos]

I think there's an easier approach, we can stop resetting the client in webkitFindControllerDispose. The find controller is created internally by the web view, and only destroyed when the view is destroyed. The controller is always given to the user as transfer none, so it's not expected to die before the web view in any case, that's why it doesn't take a reference to the view. So, there's no need to reset the find client, WebPageProxy already does that on close().

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #11)
> What's the difference? GRefPtr will not ensure the view is destroyed before
> the backend, no? is it defined the order in which things happen in
> destructors?

Yes, member variables are always constructed top-to-bottom and destroyed bottom-to-top.

(In reply to Carlos Garcia Campos from comment #12)
> I think there's an easier approach, we can stop resetting the client in
> webkitFindControllerDispose. The find controller is created internally by
> the web view, and only destroyed when the view is destroyed. The controller
> is always given to the user as transfer none, so it's not expected to die
> before the web view in any case, that's why it doesn't take a reference to
> the view. So, there's no need to reset the find client, WebPageProxy already
> does that on close().

Yes, good point. If it's safe for WebKitFindController to not keep a weak ref to the WebKitWebView -- and it is -- then it's also safe for it to not reset the client.

Maybe we should do both. Using GRefPtr is more robust because it ensures getPage() will always function properly in WPE until the WKWPE::View is destroyed. Currently, getPage() is always safe to use while WebKitWebView is being destroyed in GTK, because it works as long as WebKitWebViewBase is valid, but it's not safe in WPE. Unless we ensure that WKWPE::View is destroyed last, we'll always have this disparity between the two ports, and we'll reintroduce similar problems in the future.

[Michael Catanzaro]

So my suggestion is to take my GRefPtr patch, since that will ensure a consistent destruction order between GTK and WPE. And then...

(In reply to Carlos Garcia Campos from comment #12)
> I think there's an easier approach, we can stop resetting the client in
> webkitFindControllerDispose.

...additionally, throw in this one-line change for good measure.

[Michael Catanzaro]

Again, concern with changing only WebKitFindController is that having different order of destruction for WebKitWebView's priv members is likely to lead to other bugs in the future. Specifically, it seems the fact that getPage() can be used when finalizing the priv members in GTK but not WPE seems like quite a footgun.

Carlos, what do you think? Please pick a solution, and I'll prepare a patch accordingly.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #13)
> (In reply to Carlos Garcia Campos from comment #11)
> > What's the difference? GRefPtr will not ensure the view is destroyed before
> > the backend, no? is it defined the order in which things happen in
> > destructors?
> 
> Yes, member variables are always constructed top-to-bottom and destroyed
> bottom-to-top.

Still weak, if we forget about this and change the order of the members in the struct for whatever reason, this will break again. I prefer to explicitly destroy the things that need to happen in a specific order in the destructor.

> (In reply to Carlos Garcia Campos from comment #12)
> > I think there's an easier approach, we can stop resetting the client in
> > webkitFindControllerDispose. The find controller is created internally by
> > the web view, and only destroyed when the view is destroyed. The controller
> > is always given to the user as transfer none, so it's not expected to die
> > before the web view in any case, that's why it doesn't take a reference to
> > the view. So, there's no need to reset the find client, WebPageProxy already
> > does that on close().
> 
> Yes, good point. If it's safe for WebKitFindController to not keep a weak
> ref to the WebKitWebView -- and it is -- then it's also safe for it to not
> reset the client.
> 
> Maybe we should do both. Using GRefPtr is more robust because it ensures
> getPage() will always function properly in WPE until the WKWPE::View is
> destroyed. Currently, getPage() is always safe to use while WebKitWebView is
> being destroyed in GTK, because it works as long as WebKitWebViewBase is
> valid, but it's not safe in WPE. Unless we ensure that WKWPE::View is
> destroyed last, we'll always have this disparity between the two ports, and
> we'll reintroduce similar problems in the future.

If destroying the view is the last thing the destructor does, I don't think there will be much difference, no?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #16)
> If destroying the view is the last thing the destructor does, I don't think
> there will be much difference, no?

No, because the code you write in the destructor executes *before* the priv members are destroyed. But in GTK, the equivalent of the WKWPE::View is actually WebKitWebViewBase, which is guaranteed to be valid until all priv members have been fully destroyed.

So explicitly destroying things that need to happen in a certain order is not really possible. It *would* be possible to explicitly destroy things that need to be destroyed before the WKWPE::View, but in that case, I would say it's better to just reorder the members. It should be well-understood that the order of data members can be important in C++.

Now, if you don't want to use GRefPtr, I think the next best thing would be to sabotage getPage() so that it no longer works once the destructor has begun executing. That's the only way I can think of to ensure the order is not important, and guarantee that we won't accidentally break WPE when we introduce calls to getPage() that are safe in GTK but illegal in WPE. But that is going to be annoying: seems much nicer to ensure getPage() works for as long as possible, right? That is, I prefer to make this safe in WPE, rather than make it unsafe in GTK. By using GRefPtr, I've ensured getPage() is always safe to call until the very last priv member is destroyed.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> (In reply to Carlos Garcia Campos from comment #16)
> > If destroying the view is the last thing the destructor does, I don't think
> > there will be much difference, no?
> 
> No, because the code you write in the destructor executes *before* the priv
> members are destroyed. But in GTK, the equivalent of the WKWPE::View is
> actually WebKitWebViewBase, which is guaranteed to be valid until all priv
> members have been fully destroyed.
> 
> So explicitly destroying things that need to happen in a certain order is
> not really possible. It *would* be possible to explicitly destroy things
> that need to be destroyed before the WKWPE::View, but in that case, I would
> say it's better to just reorder the members. It should be well-understood
> that the order of data members can be important in C++.

Let me rewrite this is a less-confusing way...

Any attempt to explicitly destroy the WKWPE::View will necessarily cause it to be destroyed before all the priv members that are not explicitly destroyed. That means getPage() will be unsafe during destruction of most priv members. And that is counterproductive. Unless we change it to be unsafe in GTK as well, we'll accidentally break WPE without realizing it.

Suggestions:

 * We should either change getPage() to be safe to use during priv member destruction in WPE, or make it unsafe in GTK, so we don't accidentally break WPE without noticing by doing something that is perfectly safe in GTK.
 * If we want to make it safe in WPE, it follows that we must not explicitly destroy the WKWPE::View. Rather, we should just ensure it remains at the top of the list of priv members, so that it gets destroyed first.
 * If we instead want to make it unsafe in GTK, we can just add bool priv member and an assert to check that the priv destructor has not yet begun executing, to cause getPage() to crash when it would otherwise work perfectly fine in GTK.

[Carlos Garcia Campos]

Ok, let's use GRefPtr then.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #19)
> Ok, let's use GRefPtr then.

OK... r?

I'll plan to change the patch to stop resetting the client in webkitFindControllerDispose; that's just removing one line.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20)
> I'll plan to change the patch to stop resetting the client in
> webkitFindControllerDispose; that's just removing one line.

Er, actually it's the only line in dispose, so I'll remove all of dispose....

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20) 
> I'll plan to change the patch to stop resetting the client in
> webkitFindControllerDispose; that's just removing one line.

Actually, I think it's safer to keep that line, because:

 * This commit makes it perfectly safe to use getPage there(), so it's at worst harmless
 * Unsetting the find client is safer as it avoids potential calls to the WebKitFindController after its destruction, during the destruction of the WebKitWebView

It's a very minor point, though, so we could go either way, but keeping it seems slightly better. I'll wait until tomorrow before committing, in case you object.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #22)
> (In reply to Michael Catanzaro from comment #20) 
> > I'll plan to change the patch to stop resetting the client in
> > webkitFindControllerDispose; that's just removing one line.
> 
> Actually, I think it's safer to keep that line, because:
> 
>  * This commit makes it perfectly safe to use getPage there(), so it's at
> worst harmless
>  * Unsetting the find client is safer as it avoids potential calls to the
> WebKitFindController after its destruction, during the destruction of the
> WebKitWebView
> 
> It's a very minor point, though, so we could go either way, but keeping it
> seems slightly better. I'll wait until tomorrow before committing, in case
> you object.

It gives the wrong impression that the find controller can be alive after the web view, like we thought before investigating this issue.

[Michael Catanzaro]

OK, I'll remove it.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> No wait, it's not a great idea. WebKitWebView holds a ref on the
> WebKitFindController, so there's no reason for WebKitFindController to need
> a weak pointer. WebKitWebView should be valid for its entire lifetime. It's
> simpler to just fix the order of destruction.

^ That comment is totally wrong. The guarantee is that WebKitFindController will be valid for the lifetime of the WebKitWebView, not that WebKitWebView will be valid for the lifetime of the WebKitFindController. It's easy to see how that might not be the case, if an application refs the WebKitFindController and then unrefs the WebKitWebView.

So your suggestion in comment #4 is valid. But it's an orthogonal problem to this bug. Using the weak pointer would not help to fix this bug.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #23) 
> It gives the wrong impression that the find controller can be alive after
> the web view, like we thought before investigating this issue.

Your comment here is almost correct: unsetting the FindClient gives the wrong impression that the *WebKitWebView* can be alive after the *WebKitFindController* (which is really not possible). So I will do as you recommend, and remove the code that unsets the FindClient.

Then considering whether to use a weak pointer would be another bug. We probably should, though it would only matter if an application uses WebKitFindController in a strange and unexpected way.

[Michael Catanzaro]

Created attachment 332030 [details]
Patch

[Michael Catanzaro]

Committed r227418: <https://trac.webkit.org/changeset/227418>