409310 - Bug 315010 introduced broken page info code

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 315010 comment 17 (and bug 315010 comment 16).

In brief, the code in question can throw, but the caller doesn't catch the exception.  This can lead to a busted page info window on some pages.

I should further note that this code should never have passed sr, if this were a component with sr...

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Asaf Romano (gone)]

Can you add add a note either here (pageInfo.js) or in makeURLAbsolute on when it could throw? Are the browser.js callers safe?

Comment 3

[Robert Longson [:longsonr]]

Something like this?

          // note:  IO.newURI(aUri) will throw if aUri is not a valid URI

I suspect the nsContextMenu.js and browser.js callers are not safe but it's a bit beyond my level of javascript knowledge to tell for sure. That should be  another bug though.

Comment 4

[Asaf Romano (gone)]

I thought it's makeURLAbsolute that could throw in this context.

Comment 5

[Robert Longson [:longsonr]]

Yes, but it throws because it's internal parts throw. I would have put that note inside makeURLAbsolute. If you want one for pageInfo.js how about

// note: makeURLAbsolute will throw if either the baseURI is not a valid URI
//       or the URI formed from the baseURI and the URL is not a valid URI

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 294186 [details] [diff] [review]
patch

This is ok, but I'd still like the resolve() silliness in makeURLAbsolute go away...  It's just slowing this code down, for no good reason.

Comment 7

[Florian Quèze [:florian]]

Is there a difference between:
 IO.newURI(aUrl, null, IO.newURI(aBase)).spec;
and
 IO.newURI(aBase).resolve(aUrl);
?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I belive there can be for some URI implementations, yes.  Really, you want to just call newURI with the right base and not be calling resolve() at all.

And I'm not sure why the base URI is being carted about as a string here instead of a URI object; presumably carryover from gecko 1.8?

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comments

Boris, is  

IO.newURI(aUrl, null, IO.newURI(aBase)).spec;

the right thing to do then?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.

Comment 11

[Robert Longson [:longsonr]]

Attachment: exactly

Comment 12

[Asaf Romano (gone)]

Comment on attachment 294286 [details] [diff] [review]
exactly

exactly.

Comment 13

[Robert Longson [:longsonr]]

checked in.