1365887 - Can't open resource:///modules/ from the location bar

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[:Gijs (he/him)]

STR:

1. type "resource:///modules/ in the location bar and hit enter

ER:
load the file listing of modules

AR:
get redirected to the default search engine

Alternative STR:

1. type "resource://app/modules/" in the location bar and hit enter

ER + AR:
load the (same) file listing of modules


This is a regression, but I'm not sure when it broke. It's also broken on beta. I don't know that this is a location bar rather than a core bug, but let's start here for now.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I suspect the problem is here:
http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/components/places/UnifiedComplete.js#1598

    // Check the host, as "http:///" is a valid nsIURI, but not useful to us.
    // But, some schemes are expected to have no host. So we check just against
    // schemes we know should have a host. This allows new schemes to be
    // implemented without us accidentally blocking access to them.
    let hostExpected = new Set(["http", "https", "ftp", "chrome", "resource"]);
    if (hostExpected.has(uri.scheme) && !uri.host)
      return false;

resource://something has a host, resource:///something doesn't. Sounds like we should relax this check (by removing resource from the Set), and have a test for it, of course.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The test could probably be added to toolkit/components/places/tests/unifiedcomplete/test_visit_url.js

Comment 3

[tfe]

Hello. Can i work on this ticket?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Sure! I'll assign the bug when you attach the first patch.
For the basic introduction you can follow the guide on MDN:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
But feel free to ask questions, there's a needinfo flag at the bottom of the bug that you can set on me.

Comment 5

[tfe]

When adding the simple test:

  do_print("visit url, resource:///modules/");
  await check_autocomplete({
      search: "resource:///modules/",
    searchParam: "enable-actions",
    matches: [ makeVisitMatch("resource:///modules/", "resource:///modules/", { heuristic: true }) ]
  }); 


It throw me an error by comparing the strings:
moz-action:visiturl,{"url":"resource%3A%2F%2F%2Fmodules%2F","input":"resource%3A%2F%2F%2Fmodules%2F"}
and
resource:///modules/

Maybe you have a tip about that?

Comment 6

[tfe]

All i can see, is that NetUtil.newURI() is not returning  the expected value?

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to tfe from comment #5)
> It throw me an error by comparing the strings:
> moz-action:visiturl,{"url":"resource%3A%2F%2F%2Fmodules%2F","input":
> "resource%3A%2F%2F%2Fmodules%2F"}
> and
> resource:///modules/
> 
> Maybe you have a tip about that?

Before or after fixing the bug? since before fixing the bug it's expected you're getting a failure.
Otherwise, it's possible there's also another bug. if you launch the browser with ./mach run after fixing the code, does the location bar work as expected?

Comment 8

[tfe]

Oh, i thought the bug was already fixed, and i only had to add the test.

After building, and accessing to the uri: resource:///modules/, it searches the string to my default search engine.
I'll look at how to fix the bug too then.

Comment 9

[tfe]

Attachment: Bug 1365887 - Access resource:// urls

Review commit: https://reviewboard.mozilla.org/r/140736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140736/

Comment 10

[tfe]

With this patch the resource:///modules is now accessible.
I think i found another bug related with the resource://.
The "parent folder link" is not working at all.

May i fix it directly here, or in another ticket?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to tfe from comment #10)
> May i fix it directly here, or in another ticket?

Another ticket please, it's unrelated to the Location Bar component.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8869108 [details]
Bug 1365887 - Access resource:// urls

https://reviewboard.mozilla.org/r/140736/#review144502

It looks good, thank you!
I started some Try builds to check tests, when those are done we can autoland this.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74890c4c6bc3
Access resource:// urls r=mak

Comment 14

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b3da04fb9f6c300a31f465bac6aeafe9e464c6b6&tochange=99d514852ac5228c05301a9f1b9ad01c742b8bae
Marco Bonardo — Bug 1306639 - Searching in locationbar by typing something and pressing enter is not accounted in telemetry. r=adw

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The range is not exactly correct, in the sense that it is right based on what happens in the product, but basically bug 1306639 made the location bar respect what UnifiedComplete decides. The real bug was in unifiedComplete from far more time, probably from the beginning, but until bug 1306639 we were just ignoring this code. Btw, not that it really matters much, now it's fixed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74890c4c6bc3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Is this something that can ride the 55 train or should we consider it for backport?

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Is this something that can ride the 55 train or should we consider it for
> backport?

I don't think it's useful to uplift it, this is something mostly for us (firefox devs), not that much for the end users.