Closed Bug 1123090 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s m6+ ---

People

(Reporter: raysatiro, Assigned: billm)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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.
Seems to be e10s only can someone update the subject [e10s]. Thanks
tracking-e10s: --- → ?
Component: Untriaged → Location Bar
Version: 37 Branch → Trunk
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Location bar is missing URL → Location bar is missing URL, Opened link with target=_blank
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=68b90a5407b2&tochange=8798cd000e6b

Regressed by: Bug 567058
Blocks: 567058
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(wmccloskey)
Summary: Location bar is missing URL, Opened link with target=_blank → [e10s] Location bar is missing URL, Opened link with target=_blank
Component: Location Bar → IPC
Product: Firefox → Core
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.
Duplicate of this bug: 1123326
Assignee: nobody → wmccloskey
Flags: needinfo?(wmccloskey)
Attached patch fix-window-openSplinter Review
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.
Attachment #8552807 - Flags: review?(bent.mozilla)
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.
Attachment #8552807 - Flags: review?(bent.mozilla) → review+
Duplicate of this bug: 1124375
Duplicate of this bug: 1124534
Thanks Wes. I think the other one was the broken one.
https://hg.mozilla.org/integration/mozilla-inbound/rev/062003578db5
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/062003578db5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
QA Whiteboard: [good first verify][verify in Nightly only]
You need to log in before you can comment on or make changes to this bug.