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

VERIFIED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: rpl, Assigned: mattw)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 verified, firefox48 verified)

Details

(Whiteboard: tabs [triaged])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8711740 [details]
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

Updated

2 years ago
Assignee: nobody → mwein
(Assignee)

Comment 1

2 years ago
Created attachment 8712854 [details] [diff] [review]
Proposed changes to tabs.create to handle missing url parameter.
Attachment #8712854 - Flags: review?(kmaglione+bmo)
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.
Attachment #8712854 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

2 years ago
> 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.
Status: NEW → ASSIGNED

Updated

2 years ago
Flags: blocking-webextensions+
Priority: -- → P3
Whiteboard: tabs [triaged]

Updated

2 years ago
Flags: blocking-webextensions+
(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.
(Assignee)

Comment 5

2 years ago
> 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?
(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.
(Assignee)

Comment 7

2 years ago
Created attachment 8713951 [details] [diff] [review]
fix how tab.create handles missing URLs

HG ***
HG Bug 1242588 fix how tab.create handles missing URLs r?kmag
Attachment #8713951 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Blocks: 1244427
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.
Attachment #8713951 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8714054 [details] [diff] [review]
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.
Attachment #8714054 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8713951 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8714055 [details] [diff] [review]
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.
Attachment #8714055 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8714055 - Attachment description: fix how tab.create handles missing URLs. try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n --post-to-bugzilla bug 1242588 → fix how tab.create handles missing URLs.
Attachment #8714055 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

2 years ago
Attachment #8714054 - Attachment is obsolete: true
Attachment #8714054 - Flags: review?(kmaglione+bmo)
(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?
(Assignee)

Comment 13

2 years ago
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.
I guess it would be good to log a description of what each of the tests is testing for.
(Assignee)

Comment 15

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

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

Yep, I think so.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
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)
Flags: needinfo?(mwein)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 18

2 years ago
Created attachment 8715469 [details] [diff] [review]
fix how tab.create handles missing URLs

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4decee1a74fa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 years ago
Flags: needinfo?(mwein)
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.
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
status-firefox48: --- → verified
You need to log in before you can comment on or make changes to this bug.