64041 - LocationImpl::SetHash QIs for wrong interface

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

LocationImpl::SetHash contains the lines:

    nsIURL* url;
    result = uri->QueryInterface(NS_GET_IID(nsIURI), (void**)&url);

This is evil - you QI for nsIURI into an nsIURL* and then assume you can call
SetRef.  You're better off switching to an nsCOMPtr and using the typesafe
do_QueryInterface:

nsCOMPtr<nsIURL> url( do_QueryInterface(uri, &result) );

Comment 1

[Johnny Stenback (:jst)]

Nice catch, I have a huge patch that cleans up nsLocation.cpp and fixes this
problem, the cleanup is mostly converting the code to using nsCOMPtr's and
nsXPIDLCString's.

Comment 2

[Johnny Stenback (:jst)]

Attachment: Proposed fix and major cleanup of nsLocation.cpp

Comment 3

[Johnny Stenback (:jst)]

Looking for r= and sr=.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The only error I see that should prevent this from being checked in is in
LocationImpl::SetProperty, where you meant to replace JS_GetStringBytes
with JS_GetStringChars but did not.  (The need for a reinterpret_cast
from jschar (uint16) to wchar_t hides the fact that you used the wrong
function.)

With that fixed (and my last comment below at least examined), r=dbaron.

However, some other things I noticed that you may want to fix:

The way you find out whether NS_NewURI succeeded is inconsistent:
sometimes you null-check the URI and sometimes you check the result.
(Also you sometimes return on failure and sometimes make the rest of
function conditional on success.)

The frequent use of |nsIURI* url| rather than |nsIURI* uri| can be
confusing, especially since there are places where you deal with |nsIURI
*uri| and |nsIURL *url| in the same function.  Then again, ugly patterns
like that exist throughout the code.

The way the code is set up, it seems like calling GetHostname on
about:blank (say, in a frame) would fail but return NS_OK (since I
think about:blank could be represented as an nsSimpleURI, which doesn't
implement nsIURL, right?).  Should that be a more sensible error (e.g.,
a DOM exception)?  Or can that happen at all?  (Can an nsSimpleURI ever
end up here?  Do frames have a location object?)  Maybe at least you
should use do_QueryInterface(..., &rv)?

When you test an nsXPIDLCString as a boolean, perhaps .get() would make
things clearer, e.g. (in GetSearch):
  if (NS_SUCCEEDED(result) && search.get() && *(search.get()))
I assume it works the way you intended (or didn't think of changing),
although I'm not 100% sure.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Proposed fix #2

Comment 6

[Johnny Stenback (:jst)]

Oops! Well, the JS_GetStringBytes is fixed in the new patch, nsIURI variables
are no longer named url, error checking is more consistent. Not sure what to say
about calling GetHostname on about:blank, it should just work, there's no host
so you'll get an empty string, there's no nsIURL used in GetHostname, I'll
attach a third patch that makes sure we do throw an exception if someone tries
to *set* a property that is only available in nsIURL's and the current location
is pointing to a nsIURI.

As for testing nsXPIDL[C]String's as booleans I intentionally left those as just
if (.. && ref && *ref) since IMO nsXPIDL[C]String's are supposed to be as opaque
as they can and you should think they are just [PRUni]char pointers with the
benefit of having their own memory management, kinda like nsCOMPtr's. And yes,
it does work as I intended.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Proposed fix #3

Comment 8

[Johnny Stenback (:jst)]

Patch checked in, marking FIXED.

Comment 9

[Prashant Desale]

David Baron, do you want to verify this one ?