When I do this it starts loading Google in a new tab, then cancels Google and loads Yahoo instead.

Try it again.

If you run
  epiphany  http://www.yahoo.com/ & epiphany  http://www.google.com/ &
as one long command string, you can get either page to be the last page loaded.

I think whichever is the final loaded page depends on a context-switch race, but I don't know for sure.

Well it's definitely a bug -- it needs to load both -- but for me it pretty consistently ends on yahoo.

Created attachment 284844 [details] [review]
ephy-shell: allow multiple open_uris idles

We should not silently fail a call to ephy_shell_open_uris(), to handle
a race condition where a user clicks on multiple URIs in quick
succession.

This is a race that could cause this issue and we should probably fix it, but in practice it's not to blame.

Created attachment 284845 [details] [review]
ephy-web-view: consider page to be loading once load is requested

It's unexpected that ephy_web_view_is_loading() can return false after
ephy_web_view_load_url() is called but before the page load actually
begins. Return true instead, to plug a race where EphyShell may reuse
the EphyWebView for a new load after ephy_web_view_load_url() has been
called but before the page actually begins to load.

This is yucky, but it's the real problem here. I don't like shadowing webkit_web_view_is_loading() with another function that seems similar but gives a subtly different result, though. I wonder if we should do this same thing in webkit instead. The problem is that the web view only considers itself to be loading after the call to didStartProvisionalLoadForFrame in WebKitLoaderClient.cpp, and that doesn't happen during the call to webkit_web_view_load_uri().

Fair warning: there's a third race that I'm still looking for, but this fixes the problem about 90% of the time for me.

In the third race (please let there not be more :), ephy_shell_before_emit() in the primary instance receives the correct platform data from both remote instances, but ephy_shell_startup_continue() calls ephy_shell_open_uris() twice with the same uris. I guess this is because ephy_session_resume() is asynchronous and not guaranteed to finish before ephy_shell_before_emit() is called again in response to the second remote instance. Not sure the best way to proceed here....

Review of attachment 284844 [details] [review]:

::: src/ephy-shell.c
@@ +618,3 @@
   g_clear_object (&priv->network_monitor);
 
+  g_slist_foreach (priv->open_uris_idle_ids, remove_open_uris_idle_cb, NULL);

You should also free the list here. You could use g_slist_free_full() instead and call the g_source_remove in the destroy notify func. Otherwise the next time dispose is called you would be trying to remove the sources again.

(In reply to comment #5)
> Created an attachment (id=284845) [details] [review]
> ephy-web-view: consider page to be loading once load is requested
> 
> It's unexpected that ephy_web_view_is_loading() can return false after
> ephy_web_view_load_url() is called but before the page load actually
> begins. Return true instead, to plug a race where EphyShell may reuse
> the EphyWebView for a new load after ephy_web_view_load_url() has been
> called but before the page actually begins to load.
> 
> This is yucky, but it's the real problem here. I don't like shadowing
> webkit_web_view_is_loading() with another function that seems similar but gives
> a subtly different result, though. I wonder if we should do this same thing in
> webkit instead.

This can't be done in WebKit, because loads are not always started by the API, there are multiple ways a new load can start and we don't notice it until didStartProvisionalLoadForFrame is called.

> The problem is that the web view only considers itself to be
> loading after the call to didStartProvisionalLoadForFrame in
> WebKitLoaderClient.cpp, and that doesn't happen during the call to
> webkit_web_view_load_uri().

It happens after that call, because it's asynchronous.

> Fair warning: there's a third race that I'm still looking for, but this fixes
> the problem about 90% of the time for me.

Review of attachment 284845 [details] [review]:

::: embed/ephy-web-view.c
@@ +2361,3 @@
+  EphyWebViewPrivate *priv = view->priv;
+
+  return priv->load_requested || webkit_web_view_is_loading (WEBKIT_WEB_VIEW (view));

Maybe we could also add ephy_web_view_load_requested() or similar, and the caller can decide whether it's only interested in loading or also in loading or requested. In this case it's not confusing, becase it menas this web view requested a load, so in case of a load started by javascript or whatever this will still return false

Created attachment 284929 [details] [review]
ephy-shell: allow multiple open_uris idles

Created attachment 284930 [details] [review]
Do not lose tabs when starting Epiphany twice in quick succession

Created attachment 284932 [details] [review]
ephy-shell: take care not to lose a startup context

When starting epiphany multiple times in quick succession, storting the
startup context as a private member of EphyShell does not work because
ephy_shell_continue() could then be called multiple times with the same
startup context, causing us to lose commands and execute others multiple
times. This is because ephy_session_resume() is asynchronous and not
guaranteed to call ephy_shell_continue() before the next remote instance
triggers a call to ephy_shell_before_emit() and ephy_shell_activate().

To make sure we don't drop a startup context, invalidate the startup
context inside ephy_shell_activate() and ensure a copy of it is passed
to ephy_shell_continue().

Created attachment 284933 [details] [review]
ephy-shell: take care not to lose a startup context

Fix typo in commit message

Review of attachment 284929 [details] [review]:

::: src/ephy-shell.c
@@ +618,3 @@
   g_clear_object (&priv->network_monitor);
 
+  g_slist_free_full (priv->open_uris_idle_ids, remove_open_uris_idle_cb);

And priv->open_uris_idle_ids = NULL; since dispose can be called multiple times.

Review of attachment 284930 [details] [review]:

::: embed/ephy-web-view.c
@@ +2369,3 @@
+  EphyWebViewPrivate *priv = view->priv;
+
+  return priv->load_requested || ephy_web_view_is_loading (view);

I think this should be done by the callers, this simply returns priv->load_requested, there's still the problem of any load requested via webkit_web_view_load_* directly, though. Actually, after thinking about this again, I think this should be fixed in WebKit, the webkit documentation says:

"This property becomes TRUE as soon as a new load operation is requested and before the “load-changed” signal is emitted with WEBKIT_LOAD_STARTED and at that point the active URI is the requested one. When the load operation finishes the property is set to FALSE before “load-changed” is emitted with WEBKIT_LOAD_FINISHED."

When we added this property it was not possible to know when a new load had been requested in a web view, so we had to wait until didStartProvisionalLoadForFrame, and then emit the uri and isLoading property notifiers. But now there's a page load observer mechanism that allow us to emit the changes as soon as they happen, and the behaviour would still be the documented one (but will happen earlier).

Note also that the back forward list, for example, can also start a new request (go_back/forward) that will not be handled by this, so we should definitely handle this in WebKit.

Review of attachment 284933 [details] [review]:

::: src/ephy-shell.c
@@ +67,3 @@
+  /* Valid after ephy_shell_before_emit() or ephy_shell_set_startup_context()
+   * Invalidated during ephy_shell_activate() */
+  EphyShellStartupContext *startup_context;

I find this a bit confusing. The problem is that we can actually have two contexts, the local one and the remote one received from the calling instance. We can't have two remote ones, because emit_before and activate are called together. So, I would keep two context pointers here, local_startup_context and remote_startup_context.

@@ +130,3 @@
+
+static EphyShellStartupContext *
+ephy_shell_startup_context_copy (EphyShellStartupContext *ctx)

I don't think we need to copy the context

@@ +313,3 @@
+  EphyShell *shell;
+  EphyShellStartupContext *startup_context;
+} EphyShellStartupContextPair;

EphyShell is a single instance, we don't need a new struct just to save also the shell, since you can always get the instance with get_default().

@@ +325,2 @@
   ephy_session_resume_finish (session, result, NULL);
+  ephy_shell_startup_continue (pair->shell, pair->startup_context);

At this point you know you should be using the local startup context, since this is the primary instance loading the session.

@@ +336,3 @@
+  /* ephy_shell_activate() may be called multiple times before
+   * ephy_session_resume() finishes, so take care not to lose
+   * a startup context. */

But it's called only once for the local instance, so if at this point we have a remote startup context, we can use that one, and optimize the code below to never call ephy_session_resume(). There might be another race condition in ephy_session_resume(), since it checks whether there are windows to decide whether to load the session or not, since the session load is also async it might happen that calling resume twice in a row ended up loading the session twice.

@@ +338,3 @@
+   * a startup context. */
+  ctx = ephy_shell_startup_context_copy (shell->priv->startup_context);
+  g_clear_pointer (&shell->priv->startup_context, ephy_shell_startup_context_free);

So if there's a remote startup context we use that one instead of copying to call ephy_shell_startup_continue() directly with the remote context. We don't probably need to check the mode either, since for app mode there won't be a remote context.

@@ +909,3 @@
   g_return_if_fail (EPHY_IS_SHELL (shell));
 
+  g_clear_pointer (&shell->priv->startup_context, ephy_shell_startup_context_free);

And this is always the local one.

Created attachment 285013 [details] [review]
ephy-shell: allow multiple open_uris idles

We should not silently fail a call to ephy_shell_open_uris(), to handle
a race condition where a user clicks on multiple URIs in quick
succession.

Comment on attachment 285013 [details] [review]
ephy-shell: allow multiple open_uris idles

Attachment 285013 [details] pushed as df340df - ephy-shell: allow multiple open_uris idles

(In reply to comment #16)
> I find this a bit confusing. The problem is that we can actually have two
> contexts, the local one and the remote one received from the calling instance.
> We can't have two remote ones, because emit_before and activate are called
> together. So, I would keep two context pointers here, local_startup_context and
> remote_startup_context.

Unfortunately, we can have any number of remote contexts. If the problem was a race between the local context and a remote context, then the race would only occur if epiphany was not already open, but for my testing I had the initial window already open.

(In reply to comment #16)
> @@ +336,3 @@
> +  /* ephy_shell_activate() may be called multiple times before
> +   * ephy_session_resume() finishes, so take care not to lose
> +   * a startup context. */
> 
> But it's called only once for the local instance, so if at this point we have a
> remote startup context, we can use that one, and optimize the code below to
> never call ephy_session_resume(). There might be another race condition in
> ephy_session_resume(), since it checks whether there are windows to decide
> whether to load the session or not, since the session load is also async it
> might happen that calling resume twice in a row ended up loading the session
> twice.

Hm, I think you mean ephy_session_load() is (or should be) called only once for the local instance. ephy_session_resume() is called each time, and so I believe that session_load_cb() may be called before the next ephy_shell_before_emit().

I couldn't think of any good way to get around this except by moving ephy_session_resume() from ephy_shell_activate() to ephy_shell_startup(), which is where it really belongs since it's not related to showing the window. That will ensure it's run only once per remote instance. But then ephy_session_resume() would have to become synchronous, or else we'd have the same problem, and that would delay showing the window.

(In reply to comment #19)
> > But it's called only once for the local instance, so if at this point we have a
> > remote startup context, we can use that one, and optimize the code below to
> > never call ephy_session_resume().

Er, yes, that's right. I understand. Good idea.

(In reply to comment #20)
> (In reply to comment #19)
> > > But it's called only once for the local instance, so if at this point we have a
> > > remote startup context, we can use that one, and optimize the code below to
> > > never call ephy_session_resume().
> 
> Er, yes, that's right. I understand. Good idea.

But we still have to copy and invalidate the remote startup context in activate().

(In reply to comment #21)
> But we still have to copy and invalidate the remote startup context in
> activate().

No we don't, because we're skipping ephy_session_resume()... OK, this is good, I get it!

Created attachment 285024 [details] [review]
ephy-shell: do not lose a remote startup context

When starting epiphany multiple times in quick succession, we would
asynchronously call ephy_session_resume() from ephy_shell_activate(),
but ephy_session_resume() might not finish until after a subsequent call
to ephy_shell_activate(), causing us to lose the commands from a remote
instance and execute another's commands multiple times. Instead,
distinguish between local and remote startup contexts so that
ephy_shell_activate() can know whether we are receiving commands from a
remote instance, and not resume the session if so. This allows us to
synchronously load the commands before the next call to
ephy_shell_before_emit() invalidates the remote startup context.

Note that to test this patch, you need to first apply obsolete attachment #284930 [details], as otherwise your URIs will load in the same tab.

Review of attachment 285024 [details] [review]:

Yes, this is exactly what I meant :-)

::: src/ephy-shell.c
@@ +311,2 @@
   } else
+    ephy_shell_startup_continue (shell, priv->remote_startup_context);

I think it would be safe in this case to free the remote context here, because the emit_after and activate happen together, so that if this is the last remote context, we don't keep it around.

(In reply to comment #15)
> But now there's a page load observer mechanism that allow us to emit
> the changes as soon as they happen, and the behaviour would still be the
> documented one (but will happen earlier).

Would it happen before the call to webkit_web_view_load_uri completes? Because if not, the race would still exist.

I guess we could manually set the property to TRUE in webkit_web_view_load_uri?

(In reply to comment #24)
> I think it would be safe in this case to free the remote context here, because
> the emit_after and activate happen together, so that if this is the last remote
> context, we don't keep it around.

Yup.

Created attachment 285082 [details] [review]
ephy-shell: do not lose a remote startup context

When starting epiphany multiple times in quick succession, we would
asynchronously call ephy_session_resume() from ephy_shell_activate(),
but ephy_session_resume() might not finish until after a subsequent call
to ephy_shell_activate(), causing us to lose the commands from a remote
instance and execute another's commands multiple times. Instead,
distinguish between local and remote startup contexts so that
ephy_shell_activate() can know whether we are receiving commands from a
remote instance, and not resume the session if so. This allows us to
synchronously load the commands before the next call to
ephy_shell_before_emit() invalidates the remote startup context.

Comment on attachment 285082 [details] [review]
ephy-shell: do not lose a remote startup context

Attachment 285082 [details] pushed as 4cdae3b - ephy-shell: do not lose a remote startup context

(In reply to comment #25)
> (In reply to comment #15)
> > But now there's a page load observer mechanism that allow us to emit
> > the changes as soon as they happen, and the behaviour would still be the
> > documented one (but will happen earlier).
> 
> Would it happen before the call to webkit_web_view_load_uri completes? Because
> if not, the race would still exist.

Yes, is_loading() right after load_uri() would return TRUE. I looked at the code yesterday and it was not so easy to use the new PageLoadObserver for us, at least in its current state.

> I guess we could manually set the property to TRUE in webkit_web_view_load_uri?

Yes, a possible solution in the meantime could be to manually set is loading in all load methods, but also in go back/forward, etc. But if we manage to use PageLoadObserver, it would be much better.

(In reply to comment #28)
> Yes, a possible solution in the meantime could be to manually set is loading in
> all load methods, but also in go back/forward, etc. But if we manage to use
> PageLoadObserver, it would be much better.

I probably need this anyway for mixed content blocking exceptions. Changing the page's mixed content settings in each load and back and forward method doesn't seem very appealing either (and doesn't work if a page load starts for any other reason).

(In reply to comment #15)
> Review of attachment 284930 [details] [review]:
>
> But now there's a page load observer mechanism that allow us to emit
> the changes as soon as they happen, and the behaviour would still be the
> documented one (but will happen earlier).

Yes, I've created a WebKitPageLoadObserver class that inherits from PageLoadState::Observer, and both willChangeIsLoading() and didChangeIsLoading() are called before my call to webkit_web_view_load_uri() completes. Then didStartProvisionalLoadForFrame() is called afterwards. So I can use this to fix the race. Awesome.

OK, to sum this up:

* Two Epiphany fixes already in Epiphany 3.14.0.
* One WebKit fix will be in WebKitGTK+ 2.6.1.

Combined, they ought to resolve this issue. Thanks for reporting!