Closed Bug 1394765 Opened 7 years ago Closed 7 years ago

Remote type set by gBrowserInit.onDOMContentLoaded is bogus when restoring a session

Categories

(Firefox :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 1 obsolete file)

from bug 1388628 comment 43:

> I think bug 1351358 may also play a role here, attachment 8864117 [details] [diff] [review]
> in particular, as it updates the initial browser's
> remoteness based on the homepage URL even if we'll restore the session
> instead of loading that URL... This is quite bogus, but without bug 1365541
> (and bug 1351677 before that), it didn't matter as far as session restore
> was concerned because it wouldn't have used that tab.
Bob and Mike, what's your take on what we should do here?
Flags: needinfo?(mdeboer)
Flags: needinfo?(bobowencode)
It seems to me that if we are not loading that URL then it would be good if the correct arguments were passed on that window.
Flags: needinfo?(bobowencode)
(In reply to Bob Owen (:bobowen) from comment #2)
> It seems to me that if we are not loading that URL then it would be good if
> the correct arguments were passed on that window.

And in a less ideal world? The homepage is the default argument and session restore kicks in after that. Changing this doesn't seem feasible short-term.

What would the consequences of removing this code?
Flags: needinfo?(bobowencode)
(In reply to Dão Gottwald [::dao] from comment #3)
> (In reply to Bob Owen (:bobowen) from comment #2)
> > It seems to me that if we are not loading that URL then it would be good if
> > the correct arguments were passed on that window.
> 
> And in a less ideal world? The homepage is the default argument and session
> restore kicks in after that. Changing this doesn't seem feasible short-term.
> 
> What would the consequences of removing this code?

Actually from just testing this it looks like that part of my change never worked, it should be |typeof argToLoad === "string"| (or possibly both).

The main reason for my change was the XULElement handling when you were dragging a file:// URI tab.
I obviously didn't test this case properly.

For the standard case I think it didn't change anything (i.e. initial browser is remote) because of my JS string checking error.
Also for the normal case (with a remote homepage) even if that check was working it wouldn't have changed things.

Why do you think that this code is causing the problems in bug 1388628?

I guess we could leave the initial browser non-remote (in the non-XULElement case) but that would effectively be reverting part of the change from Bug 1261842, so you might want to check with mconley.
Flags: needinfo?(bobowencode)
(In reply to Bob Owen (:bobowen) from comment #4)
> (In reply to Dão Gottwald [::dao] from comment #3)
> > (In reply to Bob Owen (:bobowen) from comment #2)
> > > It seems to me that if we are not loading that URL then it would be good if
> > > the correct arguments were passed on that window.
> > 
> > And in a less ideal world? The homepage is the default argument and session
> > restore kicks in after that. Changing this doesn't seem feasible short-term.
> > 
> > What would the consequences of removing this code?
> 
> Actually from just testing this it looks like that part of my change never
> worked, it should be |typeof argToLoad === "string"| (or possibly both).

D'oh...

> The main reason for my change was the XULElement handling when you were
> dragging a file:// URI tab.
> I obviously didn't test this case properly.
> 
> For the standard case I think it didn't change anything (i.e. initial
> browser is remote) because of my JS string checking error.
> Also for the normal case (with a remote homepage) even if that check was
> working it wouldn't have changed things.

What's the bottom line here? Can we drop the string case handling or do you want to fix it up?

> Why do you think that this code is causing the problems in bug 1388628?

There were various factors playing into that bug. One was having your homepage set to a file:// URI, so I expected we would hit this code path.

> I guess we could leave the initial browser non-remote (in the non-XULElement
> case) but that would effectively be reverting part of the change from Bug
> 1261842, so you might want to check with mconley.

I also think bug 1397365 might obsolete some of this stuff...
Flags: needinfo?(mdeboer) → needinfo?(bobowencode)
(In reply to Do Gottwald [::dao] from comment #5)
... 
> What's the bottom line here? Can we drop the string case handling or do you
> want to fix it up?

If we don't decide to change it to leaving it non-remote, then I think we should fix it.

The similar code in _handleURIToLoad only does the typeof check, so I think that's all we need.
(This also makes me feel doubly stupid for using instanceof in the first place.)

> > Why do you think that this code is causing the problems in bug 1388628?
> 
> There were various factors playing into that bug. One was having your
> homepage set to a file:// URI, so I expected we would hit this code path.

It's possible that my change might have prevented this case then, if I hadn't been such an idiot. :-)
Attachment #8905928 - Flags: review?(dao+bmo)
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Comment on attachment 8905928 [details] [diff] [review]
Fix URL to load string type check in onDOMContentLoaded

>+      } else if (typeof argToLoad === "string") {
>         // argToLoad is String, so should be a URL.
>         remoteType = E10SUtils.getRemoteTypeForURI(argToLoad, gMultiProcessBrowser);
>         isRemote = remoteType != E10SUtils.NOT_REMOTE;

This is wrong in the session restore case where we won't load that URL (what I originally filed this bug for), so I don't think we should take this patch. I think our options are to either take session restore into account or remove this code.
Attachment #8905928 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #7)
> Comment on attachment 8905928 [details] [diff] [review]
> Fix URL to load string type check in onDOMContentLoaded
> 
> >+      } else if (typeof argToLoad === "string") {
> >         // argToLoad is String, so should be a URL.
> >         remoteType = E10SUtils.getRemoteTypeForURI(argToLoad, gMultiProcessBrowser);
> >         isRemote = remoteType != E10SUtils.NOT_REMOTE;
> 
> This is wrong in the session restore case where we won't load that URL (what
> I originally filed this bug for), so I don't think we should take this
> patch. I think our options are to either take session restore into account
> or remove this code.

Fair enough, when you said fix it up. I thought you meant the type check.
Feel free to remove it, although that won't fix anything, not sure how we take the session restore into account here.
Assignee: bobowencode → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(bobowencode)
Attachment #8905928 - Attachment is obsolete: true
Attachment #8905986 - Flags: review?(mconley)
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment on attachment 8905986 [details]
Bug 1394765 - Remove broken code that attempts to update the initial browser's remoteness based on the homepage.

https://reviewboard.mozilla.org/r/177738/#review182854

::: browser/base/content/browser.js
(Diff revision 1)
> -      } else if (argToLoad instanceof String) {
> -        // argToLoad is String, so should be a URL.
> -        remoteType = E10SUtils.getRemoteTypeForURI(argToLoad, gMultiProcessBrowser);
> -        isRemote = remoteType != E10SUtils.NOT_REMOTE;
> -      } else if (argToLoad instanceof Ci.nsIArray) {
> -        // argToLoad is nsIArray, so should be an array of URLs, set the remote
> -        // type for the initial browser to match the first one.
> -        let urisstring = argToLoad.queryElementAt(0, Ci.nsISupportsString);
> -        remoteType = E10SUtils.getRemoteTypeForURI(urisstring.data,
> -                                                   gMultiProcessBrowser);
> -        isRemote = remoteType != E10SUtils.NOT_REMOTE;
> -      }

I just want to make sure I understand this before I give r+ - is the idea here that the async `_uriToLoadPromise` stuff isn't being taken into account here?

If I understand correctly, this code is an optimization meant to help ensure that we make the initial browser the "right kind" based on what the browser is going to do soon.

Can you explain why we'd want to avoid that optimization? This could, for example, result in us creating (and then quickly destroying) a normal content process if it turns out that the page we're going to be loading is a file:// URI.
Attachment #8905986 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #11)
> I just want to make sure I understand this before I give r+ - is the idea
> here that the async `_uriToLoadPromise` stuff isn't being taken into account
> here?

Yes.

> If I understand correctly, this code is an optimization meant to help ensure
> that we make the initial browser the "right kind" based on what the browser
> is going to do soon.

But it's counter-productive when it's based on wrong assumptions.

> Can you explain why we'd want to avoid that optimization? This could, for
> example, result in us creating (and then quickly destroying) a normal
> content process if it turns out that the page we're going to be loading is a
> file:// URI.

Bug 1397365 should take care of that?
Attachment #8905986 - Flags: review?(mconley)
(In reply to Dão Gottwald [::dao] from comment #12)
> 
> Bug 1397365 should take care of that?

I'm actually not 100% sure it does. I'm not sure if inserting a remote <browser> with nodefaultsrc into a DOM will create a content process, but it seems probable based on my read of nsFrameLoader. I could very much be wrong.

In any case, I'm okay getting rid of this stuff for now, since it's broken. We might want to re-visit these optimizations again in the future, though.
Comment on attachment 8905986 [details]
Bug 1394765 - Remove broken code that attempts to update the initial browser's remoteness based on the homepage.

https://reviewboard.mozilla.org/r/177738/#review182886

Thanks!
Attachment #8905986 - Flags: review?(mconley) → review+
hg error in cmd: hg rebase -s 46d6aaa21e8d -d 5fadfed6db0c: abort: can't rebase public changeset 46d6aaa21e8d
(see 'hg help phases' for details)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a590551725dc
Remove broken code that attempts to update the initial browser's remoteness based on the homepage. r=mconley
https://hg.mozilla.org/mozilla-central/rev/a590551725dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.