After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788899 - Restore EphyWindow's navigation action decide-policy handling
Restore EphyWindow's navigation action decide-policy handling
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
unspecified
Other All
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks: 788845
 
 
Reported: 2017-10-12 19:57 UTC by Gustavo Noronha (kov)
Modified: 2017-10-24 17:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Restore EphyWindow's navigation action decide-policy handling (3.13 KB, patch)
2017-10-12 19:57 UTC, Gustavo Noronha (kov)
needs-work Details | Review
Move safe browsing verification from EphyWebView to EphyWindow (12.34 KB, patch)
2017-10-18 15:15 UTC, Gabriel Ivașcu
none Details | Review
Move safe browsing verification from EphyWebView to EphyWindow (12.38 KB, patch)
2017-10-18 16:36 UTC, Gabriel Ivașcu
committed Details | Review

Description Gustavo Noronha (kov) 2017-10-12 19:57:43 UTC
The safe browsing feature has effectively disabled other modules' handling of
decide-policy by always handling navigation action to allow async verification
to happen.

We should bypass the verification and trigger the signal again after the verification
is done, allowing other handlers to run.
Comment 1 Gustavo Noronha (kov) 2017-10-12 19:57:50 UTC
Created attachment 361453 [details] [review]
Restore EphyWindow's navigation action decide-policy handling

The safe browsing feature has effectively disabled other modules' handling of
decide-policy by always handling navigation action to allow async verification
to happen.

We now bypass the verification and trigger the signal again after the verification
is done, allowing other handlers to run.
Comment 2 Michael Catanzaro 2017-10-16 18:52:16 UTC
Review of attachment 361453 [details] [review]:

Well crap. :) That's an unfortunate regression.

::: embed/ephy-web-view.c
@@ +1325,3 @@
+     */
+    data->web_view->bypass_gsb_verification = TRUE;
+    g_signal_emit_by_name (data->web_view, "decide-policy",

Hm, I think we need to find a better way, without depending on the order that the signal handlers get connected, and without re-emitting the signal.

One option would be to just get rid of the EphyWebView handler and merge it into the EphyWindow handler. This is probably what we'll need to do.

Another would be to experiment with the order that the signal handlers run in. You could switch the EphyWebView handler to using g_signal_connect_after(), for instance. Alternatively (and probably better) you could assign a virtual implementation in class_init instead of using g_signal_connect(). Either way should cause the EphyWindow handler to run first. The likely problem with this approach is that handling the policy decision in the EphyWindow handler would cause the safe browsing check to be skipped, which could be unexpected, so it might be better to just merge everything into the EphyWindow handler. Another problem is the if (navigation_type == WEBKIT_NAVIGATION_TYPE_LINK_CLICKED) case in the EphyWindowHandler, which in the general case always calls loads the request manually with ephy_web_view_load_request() and then calls webkit_policy_decision_ignore(). We don't want that to cause the safe browsing check to be skipped.
Comment 3 Gabriel Ivașcu 2017-10-18 15:15:36 UTC
Created attachment 361812 [details] [review]
Move safe browsing verification from EphyWebView to EphyWindow
Comment 4 Michael Catanzaro 2017-10-18 16:10:25 UTC
Review of attachment 361812 [details] [review]:

This seems better, thanks. And thanks to Gustavo for the debugging.

::: embed/ephy-web-view.c
@@ +3027,3 @@
 
+gboolean
+ephy_web_view_get_bypass_safe_browsing (EphyWebView *view)

Let's name it "get_should_bypass_safe_browsing"

@@ +3035,3 @@
+
+void
+ephy_web_view_set_bypass_safe_browsing (EphyWebView *view,

set_should_bypass_safe_browsing
Comment 5 Gabriel Ivașcu 2017-10-18 16:36:54 UTC
Created attachment 361822 [details] [review]
Move safe browsing verification from EphyWebView to EphyWindow
Comment 6 Gabriel Ivașcu 2017-10-18 19:05:35 UTC
Comment on attachment 361822 [details] [review]
Move safe browsing verification from EphyWebView to EphyWindow

Attachment 361822 [details] pushed as 22c6aec - Move safe browsing verification from EphyWebView to EphyWindow
Comment 7 Gustavo Noronha (kov) 2017-10-24 17:28:45 UTC
Thanks for looking into this =)