GNOME Bugzilla – Bug 695300
[WK2] Sessions unit tests fail
Last modified: 2013-03-11 18:20:32 UTC
/src/ephy-session/load: [New Thread 0x7fffa148a700 (LWP 10977)] ** ERROR:ephy-session-test.c:116:test_ephy_session_load: assertion failed (ephy_web_view_get_address (view) == "ephy-about:memory"): ("about:blank" == "ephy-about:memory") Program received signal SIGABRT, Aborted. 0x00007ffff3fcf425 in raise () from /lib/x86_64-linux-gnu/libc.so.6 (gdb) bt
+ Trace 231600
After debugging the issue I reached the following line in ephy_web_view_load_request: // TODO: webkit_uri_request_set_uri? ephy_web_view_load_request is called from ephy_shell_new_tab_full which have the following lines: #ifdef HAVE_WEBKIT2 is_empty = ephy_embed_utils_url_is_empty (webkit_uri_request_get_uri (request)); #else is_empty = ephy_embed_utils_url_is_empty (webkit_network_request_get_uri (request)); #endif At this point, in WK2 webkit_uri_request_get_uri() is returning "about:memory". However in WK1 webkit_network_request_get_uri() is returning "ephy-about:memory". I've reported a but in WK and provided a patch in order to implement webkit_uri_request_set_uri(), once this is ready the code should be easy to fix: https://bugs.webkit.org/show_bug.cgi?id=111620
Created attachment 238254 [details] [review] Patch Attach possible patch that should be tested once WebKit bug is resolved.
(In reply to comment #1) > I've reported a but in WK and provided a patch in order to implement > webkit_uri_request_set_uri(), once this is ready the code should be easy to > fix: https://bugs.webkit.org/show_bug.cgi?id=111620 Actually this was already added in the following WK bug: https://bugs.webkit.org/show_bug.cgi?id=83681
(In reply to comment #2) > Created an attachment (id=238254) [details] [review] > Patch > > Attach possible patch that should be tested once WebKit bug is resolved. The patch didn't fix the issue :-(
(In reply to comment #4) > (In reply to comment #2) > > Created an attachment (id=238254) [details] [review] [details] [review] > > Patch > > > > Attach possible patch that should be tested once WebKit bug is resolved. > > The patch didn't fix the issue :-( That's something that I think we need to do anyway, so perhaps just open another bug and put the patch there. I wonder what's broken because of it being missing though.
The real problem of this patch is that we were checking the address without waiting for the load to be committed. Even in WK2 the load didn't started in some cases. This is more easy to check in WK2 because of the different processes, WebProcess could start the load, however UIProcess has not been notified yet, so WebKitWebView didn't emit the "load-changed" signal yet. As the address is being set in the callback for "load-changed" in ephy-web-view, we need to wait till the load is committed in order to check it. The same problem could appear in WK1, so the patch should fix the problem for both WK1 and WK2. While trying to fix the test a different issue has been detected (bug #695437), which makes previous approach to fail for some tests in WK2.
Created attachment 238369 [details] [review] Patch As explained in the Git comment the patch is generic for both WK1 and WK2. In WK2 some asserts are skipped till bug #695437 is not fixed, but they're marked with FIXME. With the patch session tests pass in WK1 and WK2 (skipping some asserts).
Created attachment 238374 [details] [review] Patch updated New version of the patch as even when all tests were passing in my machine I forgot to wait for page was being loaded in "/src/ephy-session/restore-tabs" test. All session tests should pass both in WK1 and WK2 with this patch.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > Created an attachment (id=238254) [details] [review] [details] [review] [details] [review] > > > Patch > > > > > > Attach possible patch that should be tested once WebKit bug is resolved. > > > > The patch didn't fix the issue :-( > > That's something that I think we need to do anyway, so perhaps just open > another bug and put the patch there. I wonder what's broken because of it being > missing though. I've reported a new bug related to webkit_uri_request_set_uri and I've just attached the patch there (see bug #695446).
Created attachment 238452 [details] [review] session-fix.diff Ok, here's another way of doing this. I'm making the session tell us about the newly created WebViews, so we can connect to their load process before the load starts. This allows the whole thing to work more reliably, and is somewhat simpler. The only test left to fix is the restore tabs one, because it does not use the session to load things. We can just move the signal to EphyShell and use it in all tests, I think that should work, but I'm uploading this as it is for comments. Tell me what you think.
Review of attachment 238452 [details] [review]: Yes, I agree that this solution seems safer. Now we're sure that we're connected to the load signals in the right moment. I've tested your patch (with the small changes proposed) and it works for me both in WK1 and WK2. ::: tests/ephy-session-test.c @@ +105,3 @@ +#endif + + if (status == WEBKIT_LOAD_COMMITTED || status == WEBKIT_LOAD_FINISHED) { I think it's enough to check WEBKIT_LOAD_COMMITTED, as now we're sure that load didn't start when we got connected to the signal. @@ -91,0 +92,19 @@ +static guint web_view_ready_counter = 0; + +static void ... 16 more ... I think that this should be moved out of the if. Even if there're still some views pending to be loaded, we can already disconnect the signal for the views that have already finished. Otherwise, we're only disconnecting the signal in the last view. So inside the if (when all views have been loaded) we'll only have the line: g_main_loop_quit (loop); @@ +319,3 @@ +#ifndef HAVE_WEBKIT2 + /* FIXME: This #ifndef should be removed once bug #695437 is fixed */ +// check_ephy_web_view_address (view, "ephy-about:epiphany"); Not need to comment out this line as it's already inside the #ifndef HAVE_WEBKIT2. @@ +374,3 @@ +#ifndef HAVE_WEBKIT2 + /* FIXME: This #ifndef should be removed once bug #695437 is fixed */ +// check_ephy_web_view_address (view, "ephy-about:epiphany"); The same here.
Great, I have moved the signal to the shell and things seem to work, although the tab restore test crashes (!). I'll figure it out and cleanup things a bit and upload the patches.
Created attachment 238467 [details] [review] ephy-embed-shell: add a ::web-view-created signal Emitted every time we create an EphyWebView anywhere. This will be useful in our tests.
Created attachment 238468 [details] [review] ephy-session-test: make session tests work more reliably Based on a patch by Manuel Rego. Ensure the WebViews created during the session load are fully loaded before we check their URIs. This was working more or less by pure chance in WK1, and was failing in WK2 because of the process separation. The restore tab test is skipped in WK2 for now, since it's crashing. WARNING: The patch excludes some asserts in WK2 while bug #695437 is not fixed. They are marked with FIXME.
I'm leaving the tab restore test out for now, I think this is a good improvement as it is.
Review of attachment 238467 [details] [review]: The patch looks good to me, the only problem is that the new test didn't pass in WK1 (but not sure if this is really important at this point that we're about to make WK2 default in Epiphany). Anyway, just for the record, the backtrace is the following:
+ Trace 231622
::: embed/ephy-embed-shell.c @@ +499,3 @@ + * #EphyWebView is created. + * + **/ Just a minor comment, there're trailing whitespaces in the empty lines inside the comment. Not sure if you usually worry about this things in Ephy or not.
Review of attachment 238468 [details] [review]: If we change the URL, we should change the tests to check them in the proper order, or check that the URL is any of "ephy-about:epiphany" or "ephy-about:config". ::: tests/ephy-session-test.c @@ +116,3 @@ + +static void +wait_until_load_is_committed_or_finished (WebKitWebView *web_view, GMainLoop *loop) I'd rename it to wait_until_load_is_committed as we're only waiting for WEBKIT_LOAD_COMMITTED now. @@ +206,3 @@ "</window>" "<window x=\"73\" y=\"26\" width=\"1067\" height=\"740\" active-tab=\"0\" role=\"epiphany-window-1261c786\">" + "<embed url=\"about:config\" title=\"Epiphany\"/>" If you change here the URL, the issue described in bug #695437 doesn't happen so you don't need the #ifndef HAVE_WEBKIT2. Moreover, this change makes the test breaks in WK1. We could change it here, if we're able to know the order of the views in the tests, however I'm not sure we can do it. So, I'd keep it with "about:epiphany". Anyway, with this change test_ephy_session_clear will be timeouting (as the load never finishes) so it should be skipped in WK2 too.
(In reply to comment #16) > Review of attachment 238467 [details] [review]: > > The patch looks good to me, the only problem is that the new test didn't pass > in WK1 (but not sure if this is really important at this point that we're about > to make WK2 default in Epiphany). > > Anyway, just for the record, the backtrace is the following: > > Wow, that does not make any sense! I'll have a look, but as you say I'm not sure it is very important now.
(In reply to comment #17) > Review of attachment 238468 [details] [review]: > > If we change the URL, we should change the tests to check them in the proper > order, or check that the URL is any of "ephy-about:epiphany" or > "ephy-about:config". > > ::: tests/ephy-session-test.c > @@ +116,3 @@ > + > +static void > +wait_until_load_is_committed_or_finished (WebKitWebView *web_view, GMainLoop > *loop) > > I'd rename it to wait_until_load_is_committed as we're only waiting for > WEBKIT_LOAD_COMMITTED now. > Yep. > @@ +206,3 @@ > "</window>" > "<window x=\"73\" y=\"26\" width=\"1067\" height=\"740\" active-tab=\"0\" > role=\"epiphany-window-1261c786\">" > + "<embed url=\"about:config\" title=\"Epiphany\"/>" > > If you change here the URL, the issue described in bug #695437 doesn't happen > so you don't need the #ifndef HAVE_WEBKIT2. Moreover, this change makes the > test breaks in WK1. Yeah, I changed it to make them pass, it's actually irrelevant that the URIs are different or the same. How does it fail in WK1? > > We could change it here, if we're able to know the order of the views in the > tests, however I'm not sure we can do it. So, I'd keep it with > "about:epiphany". Anyway, with this change test_ephy_session_clear will be > timeouting (as the load never finishes) so it should be skipped in WK2 too. I think we can either have them check things in the right order or check that any of them are loaded, if we do not manage to fix the WK2 bug in time.
Comment on attachment 238467 [details] [review] ephy-embed-shell: add a ::web-view-created signal The failure was due to https://bugzilla.gnome.org/show_bug.cgi?id=695620. So I have pushed the patch with the test commented out for now, we need to fix that.
(In reply to comment #19) > > @@ +206,3 @@ > > "</window>" > > "<window x=\"73\" y=\"26\" width=\"1067\" height=\"740\" active-tab=\"0\" > > role=\"epiphany-window-1261c786\">" > > + "<embed url=\"about:config\" title=\"Epiphany\"/>" > > > > If you change here the URL, the issue described in bug #695437 doesn't happen > > so you don't need the #ifndef HAVE_WEBKIT2. Moreover, this change makes the > > test breaks in WK1. > > Yeah, I changed it to make them pass, it's actually irrelevant that the URIs > are different or the same. How does it fail in WK1? Because of in WK1 we're checking: #ifndef HAVE_WEBKIT2 /* FIXME: This #ifndef should be removed once bug #695437 is fixed */ check_ephy_web_view_address (view, "ephy-about:epiphany"); #endif So, one view is "ephy-about:epiphany" but the other one is "ephy-about:config". If we're going to change the URIs, then we should change the code checking the address or skip it in WK1 too.
Comment on attachment 238468 [details] [review] ephy-session-test: make session tests work more reliably OK, what I have done in the end is have the URIs be the same for WK1 and different for WK2.
Closing the bug.