1123090 - [e10s] Location bar is missing URL, Opened link with target=_blank

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ray Satiro]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150118030202

Steps to reproduce:

Go to https://startpage.com/eng/protect-privacy.html?
Scroll to the European Privacy Seal logo a little less than halfway down the page.
Left click on the link 'European Privacy Seal'.



Actual results:

A new tab opens and loads the page but there is no throbber in the tab or URL in the location bar.


Expected results:

While the page loads the throbber should be visible.
The URL should always be visible in the location bar.

Comment 1

[Ray Satiro]

Seems to be e10s only can someone update the subject [e10s]. Thanks

Comment 2

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=68b90a5407b2&tochange=8798cd000e6b

Regressed by: Bug 567058

Comment 3

[Ray Satiro]

Something else I notice about this is the zoom setting is applied after the page is rendered. For example click the european privacy link and it opens the europrise site in a new tab. Zoom twice on the europrise page via CTRL ++. Close the europrise page and click the link again. The europrise page will open in a new tab unzoomed but then after it finishes loading the previous zoom setting is restored.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-window-open

When we make a new tab via window.open, the child sends a CreateWindow message to the parent. It creates the new tab and sets some things up. When the new <browser> element is created, nsFrameLoader tries to load about:blank into it. This gets translated into an IPC message to the child. Before bug 567058, since CreateWindow was an intr message we processed these IPC messages in the child before CallCreateWindow returned. Shortly after CallCreateWindow returned, the child would load the correct URL.

After switching CreateWindow to sync, the LoadURL message for about:blank was received after the child had already loaded its own correct URL. That was bad, so I put some code in so that we never send the about:blank load.

However, it turns out that we rely on this message. When the frontend opens the new tab, it assumes it starts as about:blank. For some reason it ignores all nsIWebProgressListener notifications until the first STATE_STOP, which it assumes will be for about:blank. Since we never load about:blank with bug 567058, we actually ignore the entire load of the first page.

I considered changing the frontend, but I'm afraid this could bust a lot of other stuff. There are tons of nsIWebProgressListeners out there in Firefox and add-ons. So I'd rather replicate the existing behavior. All we have to do is record the URL that we would have sent the load message for (about:blank) and return it as part of CreateWindow. We're already doing the same thing for frame script loading, so it's not really so bad.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8552807 [details] [diff] [review]
fix-window-open

Review of attachment 8552807 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +1593,5 @@
>        MOZ_CRASH();
>      }
>    }
>  
> +  if (urlToLoad.Length()) {

Nit: !IsEmpty()

::: dom/ipc/TabParent.cpp
@@ +459,4 @@
>    {
>      MOZ_ASSERT(!TabParent::sNextTabParent);
>      TabParent::sNextTabParent = aNewTab;
> +    aNewTab->mCreatingWindow = true;

Also, can you MOZ_ASSERT(!aNewTab->mCreatingWindow) before setting this?

@@ +459,5 @@
>    {
>      MOZ_ASSERT(!TabParent::sNextTabParent);
>      TabParent::sNextTabParent = aNewTab;
> +    aNewTab->mCreatingWindow = true;
> +    aNewTab->mDelayedURL = nsString();

Just do:

  aNewTab->mDelayedURL.Truncate()

@@ +627,5 @@
>      aURI->GetSpec(spec);
>  
> +    if (mCreatingWindow) {
> +        // Don't send the message if the child wants to load its own URL.
> +        mDelayedURL = spec;

Maybe first assert that mDelayedURL is empty? I hope this can't happen more than once.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb689ec8086

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This and the patches from bug 1123461 backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1e53e8c79d9f for Windows build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=5799592&repo=mozilla-inbound

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks Wes. I think the other one was the broken one.
https://hg.mozilla.org/integration/mozilla-inbound/rev/062003578db5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/062003578db5