1242588 - tabs.create without an URL should create a new tab pointed on the correct "about:" page and the url bar should be empty

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-about-newtab.xpi

STR:
- install the attached example extension

- in a new normal window:
  - press the browserAction button
  - EXPECTED: a new tab opened on the "about:newtab" content and the url bar should be empty
  - ACTUAL: a new tab opened on the expected content and a chrome url visible in the url bar ("chrome://browser/content/newtab/newTab.xhtml")

- in a new private window:
  - press the browserAction button
  - EXPECTED: a new tab opened on the "about:privatebrowsing" content the url bar should be empty
  - ACTUAL: a new tab opened with the "about:newtab" content instead of the expected one and the chrome url is visible in the url bar

Comment 1

[Matthew Wein [:mattw]]

Attachment: Proposed changes to tabs.create to handle missing url parameter.

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8712854 [details] [diff] [review]
Proposed changes to tabs.create to handle missing url parameter.

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

::: browser/components/extensions/ext-tabs.js
@@ +271,5 @@
> +          WindowManager.topWindow;
> +
> +        if (PrivateBrowsingUtils.isWindowPrivate(window)) {
> +          url = "about:privatebrowsing";
> +        }

I can't deny that this works, but there are two problems:

1) You're overriding the add-on-provided URL in private browsing windows. We should only be using about:privatebrowsing in that window if they don't provide a URL.

2) There's already a built-in way to decide which URL to use. Just use `window.BROWSER_NEW_TAB_URL` if `createProperties.url` is null.

Note that you're going to have to be careful how this interacts with bug 1227462. That check will prevent add-ons from loading the URL directly, so you're going to need to only check it when the add-on is providing a URL, not when the URL is null and you default to this.

Also, you're going to have to do this from within the `createInWindow` function, since the window isn't guaranteed to be initialized yet at this point.

::: browser/components/extensions/test/browser/browser_ext_tabs_create.js
@@ +64,5 @@
>                result: { url: browser.runtime.getURL("bg/blank.html") },
>              },
>              {
>                create: {},
> +              result: { url: "about:newtab" },

It would be nice to add an additional test for a private browsing window. But you can file a follow-up bug for that.

Comment 3

[Matthew Wein [:mattw]]

> 1) You're overriding the add-on-provided URL in private browsing windows. We
> should only be using about:privatebrowsing in that window if they don't
> provide a URL.
Fixed.

> 2) There's already a built-in way to decide which URL to use. Just use
> `window.BROWSER_NEW_TAB_URL` if `createProperties.url` is null.
Thanks!

> Note that you're going to have to be careful how this interacts with bug
> 1227462. That check will prevent add-ons from loading the URL directly, so
> you're going to need to only check it when the add-on is providing a URL,
> not when the URL is null and you default to this.
I'm not sure I understand this.  Will 1227462 prevent add-ons from calling 'tabs.create' with a missing URL? If so, what check should I make before modifying the URL?

> Also, you're going to have to do this from within the `createInWindow`
> function, since the window isn't guaranteed to be initialized yet at this
> point.
Done.

> ::: browser/components/extensions/test/browser/browser_ext_tabs_create.js
> @@ +64,5 @@
> >                result: { url: browser.runtime.getURL("bg/blank.html") },
> >              },
> >              {
> >                create: {},
> > +              result: { url: "about:newtab" },
> 
> It would be nice to add an additional test for a private browsing window.
> But you can file a follow-up bug for that.
Will do once I understand how this change will interact with 1227462.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Matthew Wein [:mattw] from comment #3)
> > Note that you're going to have to be careful how this interacts with bug
> > 1227462. That check will prevent add-ons from loading the URL directly, so
> > you're going to need to only check it when the add-on is providing a URL,
> > not when the URL is null and you default to this.
>
> I'm not sure I understand this.  Will 1227462 prevent add-ons from calling
> 'tabs.create' with a missing URL? If so, what check should I make before
> modifying the URL?

It will prevent them from calling it with privileged URLs that they don't have
access to load. about:newtab and about:privatebrowsing would fail those
checks, so we just need to make sure we're only checking URLs provided by the
extension, and not the ones provided by us.

> > It would be nice to add an additional test for a private browsing window.
> > But you can file a follow-up bug for that.
>
> Will do once I understand how this change will interact with 1227462.

I think that your patch will land before that one does, so I'd say not to
worry about it right now, and add a comment to that bug when yours lands.

Comment 5

[Matthew Wein [:mattw]]

> It will prevent them from calling it with privileged URLs that they don't
> have access to load. about:newtab and about:privatebrowsing would fail those
> checks, so we just need to make sure we're only checking URLs provided by the
> extension, and not the ones provided by us.

What's the difference between URLs provided by the extension and the ones provided by us?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Matthew Wein [:mattw] from comment #5)
> What's the difference between URLs provided by the extension and the ones
> provided by us?

Nothing, really. But those URLs are meant to only be loaded under specific circumstances. about:newtab when we're opening a new tab in a normal window, about:privatebrowsing when in a private browsing window. Extensions and ordinary web pages aren't allowed to load them on their own. But when we're opening a new tab without an explicit URL, those are the URLs we want to load.

Comment 7

[Matthew Wein [:mattw]]

Attachment: fix how tab.create handles missing URLs

HG ***
HG Bug 1242588 fix how tab.create handles missing URLs r?kmag

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8713951 [details] [diff] [review]
fix how tab.create handles missing URLs

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

::: browser/components/extensions/ext-tabs.js
@@ +261,5 @@
>        }).api(),
>  
>        create: function(createProperties, callback) {
> +        function createInWindow(window) {
> +          let url = createProperties.url || window.BROWSER_NEW_TAB_URL;

We still need to resolve the URL relative to the current context when it's provided, so something like:

let url;
if (createProperties.url !== null) {
  url = context.resolve(createProperties.url);
} else {
  url = window.BROWSER_NEW_TAB_URL;
}

I'm pretty sure some tests will fail without that.

Comment 9

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab1d83ef00ee

Comment 10

[Matthew Wein [:mattw]]

Attachment: fix how tab.create handles missing URLs.

Good catch.  I noticed that one test was failing (for http://www.blank.html/) but it looked like it didn't relate to the changes I was making. I think it could make debugging easier if we name the tests and log them with each result.

Comment 11

[Matthew Wein [:mattw]]

Attachment: fix how tab.create handles missing URLs.

Good catch. I noticed that one test was failing (for blank.html) but it looked like it didn't relate to the changes I made. I think it would've helped me if the tests had names that we logged with each result.  If you agree I could work on that in a follow up bug.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Matthew Wein [:mattw] from comment #11)
> I think it would've helped me if the tests had names that we logged with
> each result. If you agree I could work on that in a follow up bug.

I'm not sure what you mean. Which tests exactly are you talking about?

Comment 13

[Matthew Wein [:mattw]]

So the main issue I had was that I found it difficult to tell how the changes I made were causing a test to fail (the second test in browser_ext_tabs_create.js). In the past I'm used to unit tests having names, like  |TestCreate_WithRelativeURL_ShouldResolveToCurrentContext()| which can help make it easier to know exactly what's being tested. However, I'm not saying that this is absolutely necessary, and I think this might just mean that I need to spend more time getting to know the code better.

Comment 14

[Kris Maglione [:kmag]]

I guess it would be good to log a description of what each of the tests is testing for.

Comment 15

[Matthew Wein [:mattw]]

Okay, I can create a follow-up bug for it then.  

Is this change ready to be checked-in?

Comment 16

[Kris Maglione [:kmag]]

(In reply to Matthew Wein [:mattw] from comment #15)
> Is this change ready to be checked-in?

Yep, I think so.

Comment 17

[Carsten Book [:Tomcat]]

fails to apply:

patching file browser/components/extensions/test/browser/browser_ext_tabs_create.js
Hunk #1 FAILED at 45
1 out of 1 hunks FAILED -- saving rejects to file browser/components/extensions/test/browser/browser_ext_tabs_create.js.rej
patch failed, unable to continue (try -v)

Comment 18

[Matthew Wein [:mattw]]

Attachment: fix how tab.create handles missing URLs

Comment 19

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/4decee1a74fa58ee183496d512efb534e71250ce
Bug 1242588 fix how tab.create handles missing URLs. r=kmag

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4decee1a74fa

Comment 21

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 46.0a1 (2016-01-25) under Windows 10 64-bit.

Verified fixed on Firefox 48.0a1 (2016-04-03/04) and Firefox 47.0a2 (2016-04-04) under Windows 10 64-bit, Mac OS X 10.11.2 and Ubuntu 12.04 32-bit.