1412656 - Firefox breaks/malforms URLs of some bookmarks

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Create folder for bookmarks
2. Create 2 bookmarks from following data in that created folder:

name
> "Script for Win 7/8 to block all telemetry updates and Windows 10 upgrade components | technology" - Szukaj w Google
location
> https://www.google.pl/search?q=%22Script+for+Win+7%2F8+to+block+all+telemetry+updates+and+Windows+10+upgrade+components+|+technology%22

name
> "Static tRead Value" - Szukaj w Google
location
> https://www.google.pl/search?q=%22Static+tRead+Value%22

3. Select these 2 bookmarks
4. Open mouse menu
5. Select "Open All in Tabs" in opened mouse menu
and see that 3 tabs are opened with these URLs:
> https://www.google.pl/search?q=%22Script+for+Win+7%2F8+to+block+all+telemetry+updates+and+Windows+10+upgrade+components+
> http://www.+technology%22.com/
> https://www.google.pl/search?q=%22Static+tRead+Value%22
instead of only 2 with proper URLs

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: bookmarks.html

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Better STR:
1. Open Firefox Library (Ctrl+Shift+B)
2. Press "Import and Backup" button to see its menu
3. Press "Import Bookmarks from HTML..." option
4. Select "bookmarks.html" file (downloaded before from this bug Attachments)
5. Close all Firefox tabs and Windows (so bug will be always reproducible)
6. Go to new imported folder with bookmarks
7. Open them by "Open All in Tabs" option, either by selecting folder or by selecting all tabs
and see that some URLs are malformed/broken

I suspect that it's caused by this symbol in URL:
> |

Comment 3

[Drew Willcoxon :adw]

Hmm, I can't reproduce the problem on a new profile.  I tried the STR both in comment 0 and comment 2.

Comment 4

[Marco Bonardo [:mak]]

I also cannot reproduce the problem as reported. Has this been fixed in Nightly? could you please try identifying what fixed it with mozregression?

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: screencast.mp4

It's still reproducible in today and latest Mozilla Firefox Nightly 58.0a1 (2017-10-30) (64-bit) on Windows 7 (64-bit). Same as it was reproducible in yesterday Nightly. Nothing fixed it in today build, yet.

I'm attaching screencast from #5 in STR.

So make sure, that you're closing all Firefox tabs and windows, leaving only Library open, before doing STR, as said in #5 in STR.

Comment 6

[Marco Bonardo [:mak]]

Ah, then it's http://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/browser/components/places/PlacesUIUtils.jsm#923

Comment 8

[Mark Banner (:standard8)]

I'd be happy to mentor this bug. I think the right way to fix this would be to change the code pointed to in comment 6 to make a second nsIMutableArray which contains the individual URI strings (rather than join()ing them).

This would then use the array handling code for handling arguments which would avoid the .split("|") that happens in the loadOneOrMoreURIs call:

https://searchfox.org/mozilla-central/rev/3fd53c47864fedb916f0ed8f002f15456324f729/browser/base/content/browser.js#2356
https://searchfox.org/mozilla-central/rev/3fd53c47864fedb916f0ed8f002f15456324f729/browser/base/content/browser.js#2166

This should also have a new test that tests for opening a url (use something like https://example.com?foo=1|2|3) and checking a window opens with just that URL and just one tab.

It could be based around some of the following ones:

https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_library_open_bookmark.js
https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_sidebar_open_bookmarks.js

For waiting for the new window, this function should be used

Comment 9

[aarushivij]

Hello, Can I try working on this bug?
Thanks :)
Aarushi

Comment 10

[aarushivij]

Sorry, I did not realize it was an error on windows machine.
And I am working on linux.
Sorry for the inconvinience

Comment 11

[Mark Banner (:standard8)]

Hi Aarushi, there shouldn't be anything platform-dependent about this bug, so you should be able to work on it on Linux just fine. Let me know if you'd like to take it up.

Comment 12

[aarushivij]

Okay Mark
Shall take up this isse :)
Thanks
Aarushi

Comment 13

[Mark Banner (:standard8)]

Great, I'll assign it to you, let me know if you have any questions or need help. You can also ask on Matrix in the Introduction channel.

Comment 14

[Mark Banner (:standard8)]

Unfortunately I've not heard from Aarushi, so opening this up.

Comment 15

[thedrew88]

Hi Mark,
I'd like to be assigned this bug if possible. This would be my first opensource contribution, and I could certainly use the mentorship that you've offered. The code base was pretty intimidating at first, but I've been looking it over for a while now and I think I'm starting to get a decent handle on the issue. Thanks

Comment 16

[Andrew Park]

Hi, since this is still unassigned I would like to pick it up. I have a draft for a fix I will submit shortly. Thanks

Comment 17

[Andrew Park]

Attachment: Bug 1412656 - Use array instead of joined string to load bookmarks. r=standard8

Comment 18

[Andrew Park]

I'm having some trouble writing a test for this case.. Will put up patch including the test when it's done.

The test needs to do the following:

1. Open bookmark library
2. Close all open browser windows excluding library
3. Create a bookmark in some folder with an URL containing '|' e.g. example.com?foo=1|2
4. Right click folder to bring up context menu and click 'Open all in tabs'
5. Check that only one tab is opened in a new window

So far I've only managed to get step 1 working, still looking through some other tests in browser/components/places/tests which hopefully has similar workflows to what I need here. Also can't get debugging to work on my test.. GDB won't stop at the breakpoints I set for some reason

Comment 19

[Mark Banner (:standard8)]

(In reply to Andrew Park from comment #18)

2. Close all open browser windows excluding library

You shouldn't need to do this, you can just expect a new window to open - see BrowserTestUtils.waitForNewWindow(). https://searchfox.org/mozilla-central/search?q=symbol:%23waitForNewWindow&redirect=false

Or if you want an empty window you can just open a new one.

3. Create a bookmark in some folder with an URL containing '|' e.g. example.com?foo=1|2

You could probably just do this with PlacesUtils.bookmarks.insert(...) which is what a lot of the tests do. Then you only need to select it, e.g. https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_library_open_bookmark.js

4. Right click folder to bring up context menu and click 'Open all in tabs'

Similar functionality: https://searchfox.org/mozilla-central/rev/4fe8e4848664a182de3a0dbd781f93ae402012ab/browser/components/places/tests/browser/browser_library_bookmark_pages.js#31-46

5. Check that only one tab is opened in a new window

If in the waitForNewWindow you pass the url. Once that has loaded, you should be able to fairly easily check the number of tabs.

So far I've only managed to get step 1 working, still looking through some other tests in browser/components/places/tests which hopefully has similar workflows to what I need here. Also can't get debugging to work on my test.. GDB won't stop at the breakpoints I set for some reason

For javascript, you probably more want the Javascript debugger. You can kick that in with ./mach mochitest --jsdebugger path/to/test - You'll want to include a debugger; statement where you want it to pause the test.

Comment 20

[Andrew Park]

(In reply to Mark Banner (:standard8) from comment #19)

Thanks Mark for all the tips! With that I can really get started on writing the test. However in regards to this:

You shouldn't need to do this, you can just expect a new window to open - see BrowserTestUtils.waitForNewWindow(). https://searchfox.org/mozilla-central/search?q=symbol:%23waitForNewWindow&redirect=false

I think closing all windows is necessary. This bug is only reproducible if all windows are closed before you click "Open in all tabs." If there is an existing browser window already open (even an empty one) and you click "Open in all tabs," it will just open new tabs in the existing window instead of a new window, and it takes a different code path to load the URLs that doesn't have this problem.

Comment 21

[Andrew Park]

So I have everything working except this step:

Close all open browser windows excluding library

I've noticed that if I do this the test just stops running. I'm thinking maybe that since this is equivalent to restarting Browser chrome tests can't handle this case? Maybe I'll look into Marionette instead.

If I just open the bookmarks in an existing window everything works but unfortunately that doesn't really test this particular case..

Comment 22

[Mark Banner (:standard8)]

(In reply to Andrew Park from comment #21)

So I have everything working except this step:

Close all open browser windows excluding library

I've noticed that if I do this the test just stops running. I'm thinking maybe that since this is equivalent to restarting Browser chrome tests can't handle this case? Maybe I'll look into Marionette instead.

I think I've just found an alternative that runs the same code path - when right-click and clicking Open All in Tabs, on the click press shift. That forces opening in a new window, which reproduces the bug.

Here's some example code for that: https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js#116-119

Comment 23

[Andrew Park]

(In reply to Mark Banner (:standard8) from comment #22)

I think I've just found an alternative that runs the same code path - when right-click and clicking Open All in Tabs, on the click press shift. That forces opening in a new window, which reproduces the bug.

Awesome, Mark. That was exactly what I needed thank you. I've put up a test using this method. Please have a look at the latest patch thanks!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2dcac3a2399
Use array instead of joined string to load bookmarks. r=Standard8

Comment 25

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c2dcac3a2399

Comment 26

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 80.0a1 (2020-07-04).
Thank you very much for fixing this very long standing bug! \o/