Create WebKitFrameWrapper if it doesn't exist

[varumugam123]

We see that with wpe-2.38 (07fdc0799c), when the browser instance is created a warning message as below is logged

gboolean webkit_frame_is_main_frame(WebKitFrame*): assertion 'WEBKIT_IS_FRAME(frame)' failed

On further check it is observed that, post https://github.com/WebKit/WebKit/commit/4a9e02aa3418af0c2117db77f0e7edd70955fa23 commit, it is possible to emit DID_START_PROVISIONAL_LOAD_FOR_FRAME signal with a null "webKitFrame" from https://github.com/WebPlatformForEmbedded/WPEWebKit/blob/wpe-2.38/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp#L198 (because at that moment webkitFrameGet() could return nullptr).

Replacing webkitFrameGet() with webkitFrameGetOrCreate() to be in sync with wpe-2.28 and to correctly emit signal with a valid frame.

[varumugam123]

Hi Miguel, Thanks for checking this.

The suggested change would prevent the warning by not sending the signal. This means that upper layer doesn’t get notified on the main frame’s provisional navigation.

From: Miguel Gómez @.> Sent: Monday, July 31, 2023 3:32 PM To: WebPlatformForEmbedded/WPEWebKit @.> Cc: Arumugam, Vivek @.>; Author @.> Subject: Re: [WebPlatformForEmbedded/WPEWebKit] Create WebKitFrameWrapper if it doesn't exist (PR #1130)

@magomez commented on this pull request.

---

In Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpphttps://urldefense.com/v3/__https:/github.com/WebPlatformForEmbedded/WPEWebKit/pull/1130*discussion_r1279075899__;Iw!!CQl3mcHX2A!B4Ou-AueXRxd1TWOfVGpTN7UB05ysucmbfR4qdOxzrxW36PCHRPrDYkarv3y-Y09HFUaAECsf6po4o7yyCTkWshsv6dkfaY$:

@@ -195,64 +195,56 @@ class PageLoaderClient final : public API::InjectedBundle::PageLoaderClient {

 void didStartProvisionalLoadForFrame(WebPage&, WebFrame& frame, RefPtr<API::Object>&) override

 {

• auto* webKitFrame = webkitFrameGet(&frame);

• if (!webKitFrame && !frame.isMainFrame())

I think the whole problem comes from an error in this condition. I think it should be if (!webKitFrame || !frame.isMainFrame()) and it should fix the problem. Can you check whether that fixes the issue for you? There are other instances of this same condition that should be fixed as well. I can do that if you confirm that this works for you.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/WebPlatformForEmbedded/WPEWebKit/pull/1130*pullrequestreview-1554414318__;Iw!!CQl3mcHX2A!B4Ou-AueXRxd1TWOfVGpTN7UB05ysucmbfR4qdOxzrxW36PCHRPrDYkarv3y-Y09HFUaAECsf6po4o7yyCTkWshssUQypuw$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AHAQH4QMXTH7EZ6JQZ7JB63XS57BTANCNFSM6AAAAAA23PTMB4__;!!CQl3mcHX2A!B4Ou-AueXRxd1TWOfVGpTN7UB05ysucmbfR4qdOxzrxW36PCHRPrDYkarv3y-Y09HFUaAECsf6po4o7yyCTkWshsFUDD00E$. You are receiving this because you authored the thread.Message ID: @.**@.>>

[varumugam123]

The change I proposed too has some minor error that now, the signals (or even the URI update) is done only for main frames (due to the early check). I will correct it shortly...

[magomez]

I'm going to look into the consequences of using webkitFrameGetOrCreate instead of webkitFrameGet. If creating the frame at that point is not a problem this can be merged. But please, merge both commits into a single one. It doesn't make sense to keep them separated.

[varumugam123]

I'm going to look into the consequences of using webkitFrameGetOrCreate instead of webkitFrameGet. If creating the frame at that point is not a problem this can be merged.

Thanks!

But please, merge both commits into a single one. It doesn't make sense to keep them separated.

Done

[magomez]

@varumugam123 it seems that using webkitFrameGetOrCreate is fine. But the change should only be needed for didStartProvisionalLoadForFrame, which is where we need to have a WebKitFrame to use for the signal. We shouldn't need to force the creation of the WebKitFrame for the other cases that you modified. Did you find any problem with those as well? Or did you perform the same change on every function just to keep the coherency? If there are no concrete problems detected for the other functions, it would be better to leave them as they are, and fix only the problematic case.

[varumugam123]

Right, didStartProvisionalLoadForFrame() is the only place which emits a signal and of concern. I carry forwarded the change over to the other functions just to be consistent with didStartProvisionalLoadForFrame (with wpe-2.28 as well).

But I can restrict the change only to didStartProvisionalLoadForFrame(), will update the PR shortly