356571 - loadOneOrMoreURIs gives up if one of the URLs has an unknown protocol

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

The fix for bug 245588 made sure that we kept trying if one URL in the list was invalid. This was changed in bug 308396, because the individual tab loading was moved to loadTabs, which gives up once it finds an invalid URL (one for which loadURI throws). Luckily, the call to loadTabs was kept in the try/catch, so the "break startup" part of bug 245588 hasn't regressed.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I'll take this so I don't forget it, but please feel free to take this from me if you're interested in fixing it :)

Comment 3

[Marco Bonardo [:mak]]

Attachment: Add try catch clauses in loadTabs

moving patch here from Bug 390592

this adds try catch into loadTabs, to it continues if an exception is thrown

Gavin, can this be useful?

Comment 4

[Ria Klaassen (not reading all bugmail)]

I see a similar problem happen with bookmarks.
 
If the first bookmark points to a non-existing page on your disk and you right-click on this folder and choose "Open All in Tabs" it will show the Confirm Open dialog "You are about to open 26 tabs.." etc.
 
But when you confirm, it will only load one page, the "File not found" page.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 275320 [details] [diff] [review]
Add try catch clauses in loadTabs

You shouldn't need the try/catch around the addTab calls given the landed patch for bug 366705.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Marco, do you want to take this?

Comment 7

[Marco Bonardo [:mak]]

i'm not sure on what still has to be done here. i'll do some testing and see if i can do something useful. will take from you if i have some good result (and time) in the next days

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

This will not block the final release of Firefox 3. Any patch will need unit tests in order to be approved.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I think we just need attachment 275320 [details] [diff] [review], but without the try/catch around addTab, and a browser-chrome test that will call loadOneOrMoreURIs directly.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Gavin, sorry for being damn late.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 358014 [details] [diff] [review]
patch v1.1

>diff --git a/browser/base/content/test/browser_bug356571.js b/browser/base/content/test/browser_bug356571.js

>+// Save original prompt service factory

>+let fakePromptServiceFactory = {

>+  // Unregister the factory so we do not leak

>+  // Restore the original factory
>+  Cm.QueryInterface(Ci.nsIComponentRegistrar)
>+    .registerFactory(Components.ID(kPromptServiceUUID), "Prompt Service",
>+                     kPromptServiceContractID, kPromptServiceFactory);

Clever! I'm a bit worried that this might cause some test failures (e.g. due to not implementing all of promptservice, or whatever), though... will have to keep an eye on it I guess.

>diff --git a/browser/base/content/test/bug386835.html b/browser/base/content/test/bug386835.html
>deleted file mode 100644

>diff --git a/browser/base/content/test/dummy_page.html b/browser/base/content/test/dummy_page.html
>new file mode 100644

Could have just used |hg mv| for a smaller diff here, fwiw.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> Clever! I'm a bit worried that this might cause some test failures (e.g. due to
> not implementing all of promptservice, or whatever), though... will have to
> keep an eye on it I guess.

indeed in a previous version i was using removeAllTabsBut, it was firing the "close tabs" dialog through nsPromptService and failing, that's the reason i looked how to restore the original factory, indeed after the restore it was working again correctly... Ideally unless we use any method that will use that service in this test while it's overridden should not cause issues.

> Could have just used |hg mv| for a smaller diff here, fwiw.
well yeah, still i've also changed the contents so the gain would have been really minimal.

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/e369a4bf546c

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 358014 [details] [diff] [review]
patch v1.1

asking approval, low risk, comes with a test

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 358014 [details] [diff] [review]
patch v1.1

a191=beltzner

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/83264f80ea6d