647570 - Clean up nsSimpleURI inheritance in nsJSProtocolHandler

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There are a variety of URI implementations that "inherit" from nsSimpleURI, but do this at runtime.  This is because back before libxul was the one true build configuration nsSimpleURI was only guaranteed to be exposed by Necko through XPCOM.  This leads to code like http://mxr.mozilla.org/mozilla-central/source/dom/src/jsurl/nsJSProtocolHandler.h#93 which inherits from nsSimpleURI at runtime through XPCOM.

This should all die.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'd be happy to review patches here!

Comment 2

[Emanuele Costa]

Ok, will work on this.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's great Emanuele.  Feel free to ask Boris or me any questions you run into along the way.

Comment 4

[Emanuele Costa]

Attachment: proposed patch to inherits from nsSimpleURI

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 525654 [details] [diff] [review]
proposed patch to inherits from nsSimpleURI

>diff --git a/dom/src/jsurl/nsJSProtocolHandler.cpp b/dom/src/jsurl/nsJSProtocolHandler.cpp
>--- a/dom/src/jsurl/nsJSProtocolHandler.cpp
>+++ b/dom/src/jsurl/nsJSProtocolHandler.cpp
>@@ -1214,20 +1215,17 @@ nsJSProtocolHandler::NewURI(const nsACSt
>                             nsIURI **result)
> {
>     nsresult rv;
> 
>     // javascript: URLs (currently) have no additional structure beyond that
>     // provided by standard URLs, so there is no "outer" object given to
>     // CreateInstance.
> 
>-    nsCOMPtr<nsIURI> url = do_CreateInstance(NS_SIMPLEURI_CONTRACTID, &rv);
>-
>-    if (NS_FAILED(rv))
>-        return rv;
>+    nsJSURI* url = new nsJSURI(aBaseURI); 
> 
>     if (!aCharset || !nsCRT::strcasecmp("UTF-8", aCharset))
>       rv = url->SetSpec(aSpec);
>     else {
>       nsCAutoString utf8Spec;
>       rv = EnsureUTF8Spec(PromiseFlatCString(aSpec), aCharset, utf8Spec);
>       if (NS_SUCCEEDED(rv)) {
>         if (utf8Spec.IsEmpty())
>@@ -1236,21 +1234,22 @@ nsJSProtocolHandler::NewURI(const nsACSt
>           rv = url->SetSpec(utf8Spec);
>       }
>     }
> 
>     if (NS_FAILED(rv)) {
>         return rv;
>     }
> 
>-    *result = new nsJSURI(aBaseURI, url);
>-    NS_ENSURE_TRUE(*result, NS_ERROR_OUT_OF_MEMORY);
>+        
>+    NS_ENSURE_TRUE(url, NS_ERROR_OUT_OF_MEMORY);

You've already dereferenced url above. Fortunately, new is infallible, so this check can go away entirely.

> 
>+    *result = url;
>     NS_ADDREF(*result);

I would prefer

nsCOMPtr<nsIURI> url = new ...
...
url.forget(result);

And lose the explicit addref.

>-    return rv;
>+    return rv;    

You're adding trailing whitespace here. Please fix and check the rest of the patch.

> }
> 
> NS_IMETHODIMP
> nsJSProtocolHandler::NewChannel(nsIURI* uri, nsIChannel* *result)
> {
>     nsresult rv;
>     nsJSChannel * channel;
> 
>@@ -1277,169 +1276,104 @@ nsJSProtocolHandler::AllowPort(PRInt32 p
>     // don't override anything.  
>     *_retval = PR_FALSE;
>     return NS_OK;
> }
> 
> ////////////////////////////////////////////////////////////
> // nsJSURI implementation
> 
>-NS_IMPL_ADDREF(nsJSURI)
>-NS_IMPL_RELEASE(nsJSURI)
>+NS_IMPL_ADDREF_INHERITED(nsJSURI, nsSimpleURI)
>+NS_IMPL_RELEASE_INHERITED(nsJSURI, nsSimpleURI)
> 
> NS_INTERFACE_MAP_BEGIN(nsJSURI)
>-  NS_INTERFACE_MAP_ENTRY(nsIURI)
>-  NS_INTERFACE_MAP_ENTRY(nsISerializable)
>-  NS_INTERFACE_MAP_ENTRY(nsIClassInfo)
>-  NS_INTERFACE_MAP_ENTRY(nsIMutable)
>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIURI)
>   if (aIID.Equals(kJSURICID))
>-      foundInterface = static_cast<nsIURI*>(this);
>+     foundInterface = static_cast<nsIURI*>(this);
>   else
>-NS_INTERFACE_MAP_END
>+NS_INTERFACE_MAP_END_INHERITING(nsSimpleURI)
>+
> 
> // nsISerializable methods:
> 
> NS_IMETHODIMP
> nsJSURI::Read(nsIObjectInputStream* aStream)
> {
>-    nsresult rv;
>-
>-    rv = aStream->ReadObject(PR_TRUE, getter_AddRefs(mSimpleURI));
>+    nsresult rv = nsSimpleURI::Read(aStream);
>     if (NS_FAILED(rv)) return rv;
> 
>-    mMutable = do_QueryInterface(mSimpleURI);
>-    NS_ENSURE_TRUE(mMutable, NS_ERROR_UNEXPECTED);
>-
>-    PRBool haveBase;
>-    rv = aStream->ReadBoolean(&haveBase);
>+    rv = aStream->ReadObject(PR_TRUE, getter_AddRefs(mBaseURI));
>     if (NS_FAILED(rv)) return rv;
> 
>-    if (haveBase) {
>-        rv = aStream->ReadObject(PR_TRUE, getter_AddRefs(mBaseURI));
>-        if (NS_FAILED(rv)) return rv;
>+    return rv;
>+}
>+
>+NS_IMETHODIMP
>+nsJSURI::Write(nsIObjectOutputStream* aStream)
>+{
>+    

Lose this line.

>+  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(mBaseURI);
>+    if (!serializable) {
>+        // We can't serialize ourselves
>+        return NS_ERROR_NOT_AVAILABLE;
>+    }
>+
>+    nsresult rv = nsSimpleURI::Write(aStream);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    rv = aStream->WriteCompoundObject(mBaseURI, NS_GET_IID(nsIURI),
>+                                      PR_TRUE);
>+    return rv;
>+} 

More trailing whitespace on the last line, and please use two-space indentation throughout.

>+
>+
>+// nsIURI methods:
>+
>+NS_IMETHODIMP
>+nsJSURI::Equals(nsIURI* other, PRBool *result)
>+{
>+    *result = PR_FALSE;
>+
>+    if(other)
>+    {
>+        nsSimpleURI::Equals(other, result);
>+        if(PR_FALSE == *result)

if (!*result)

>+            return NS_OK;
>+
>+        nsRefPtr<nsJSURI> jsURI;
>+        nsresult rv = other->QueryInterface(kJSURICID,
>+                                       getter_AddRefs(jsURI));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        
>+        nsIURI* otherBaseURI = jsURI->GetBaseURI();
>+        
>+        if(mBaseURI)
>+            return mBaseURI->Equals(otherBaseURI, result);
>+        
>+        otherBaseURI == nsnull ? *result = PR_TRUE : *result = PR_FALSE; 

*result = !otherBaseURI;

>+            
>     }
> 
>     return NS_OK;
> }
> 
>-NS_IMETHODIMP
>-nsJSURI::Write(nsIObjectOutputStream* aStream)
>+/* virtual */ nsSimpleURI*
>+nsJSURI::StartClone()
> {
>-    nsresult rv;
>-
>-    rv = aStream->WriteObject(mSimpleURI, PR_TRUE);
>-    if (NS_FAILED(rv)) return rv;
>-
>-    rv = aStream->WriteBoolean(mBaseURI != nsnull);
>-    if (NS_FAILED(rv)) return rv;
>-
>-    if (mBaseURI) {
>-        rv = aStream->WriteObject(mBaseURI, PR_TRUE);
>-        if (NS_FAILED(rv)) return rv;
>+    NS_ENSURE_TRUE(mBaseURI, nsnull);
>+    
>+    nsCOMPtr<nsIURI> innerClone;
>+    nsresult rv = mBaseURI->Clone(getter_AddRefs(innerClone));
>+    if (NS_FAILED(rv)) {
>+        return nsnull;
>     }
> 
>-    return NS_OK;
>+    nsJSURI* url = new nsJSURI(innerClone);
>+    
>+    return url;

return new nsJSURI(innerClone);

> }
> 
>-// nsIURI methods:
>-
>-NS_IMETHODIMP
>-nsJSURI::Clone(nsIURI** aClone)
>-{
>-    nsCOMPtr<nsIURI> simpleClone;
>-    nsresult rv = mSimpleURI->Clone(getter_AddRefs(simpleClone));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    nsCOMPtr<nsIURI> baseClone;
>-    if (mBaseURI) {
>-        rv = mBaseURI->Clone(getter_AddRefs(baseClone));
>-        NS_ENSURE_SUCCESS(rv, rv);
>-    }
>-
>-    nsIURI* newURI = new nsJSURI(baseClone, simpleClone);
>-    NS_ENSURE_TRUE(newURI, NS_ERROR_OUT_OF_MEMORY);
>-
>-    NS_ADDREF(*aClone = newURI);
>-    return NS_OK;
>-}
>-
>-NS_IMETHODIMP
>-nsJSURI::Equals(nsIURI* aOther, PRBool *aResult)
>-{
>-    if (!aOther) {
>-        *aResult = PR_FALSE;
>-        return NS_OK;
>-    }
>-    
>-    nsRefPtr<nsJSURI> otherJSUri;
>-    aOther->QueryInterface(kJSURICID, getter_AddRefs(otherJSUri));
>-    if (!otherJSUri) {
>-        *aResult = PR_FALSE;
>-        return NS_OK;
>-    }
>-
>-    return mSimpleURI->Equals(otherJSUri->mSimpleURI, aResult);
>-}
>-
>-// nsIClassInfo methods:
>-NS_IMETHODIMP 
>-nsJSURI::GetInterfaces(PRUint32 *count, nsIID * **array)
>-{
>-    *count = 0;
>-    *array = nsnull;
>-    return NS_OK;
>-}
>-
>-NS_IMETHODIMP 
>-nsJSURI::GetHelperForLanguage(PRUint32 language, nsISupports **_retval)
>-{
>-    *_retval = nsnull;
>-    return NS_OK;
>-}
>-
>-NS_IMETHODIMP 
>-nsJSURI::GetContractID(char * *aContractID)
>-{
>-    // Make sure to modify any subclasses as needed if this ever
>-    // changes.
>-    *aContractID = nsnull;
>-    return NS_OK;
>-}
>-
>-NS_IMETHODIMP 
>-nsJSURI::GetClassDescription(char * *aClassDescription)
>-{
>-    *aClassDescription = nsnull;
>-    return NS_OK;
>-}
>-
>-NS_IMETHODIMP 
>-nsJSURI::GetClassID(nsCID * *aClassID)
>-{
>-    // Make sure to modify any subclasses as needed if this ever
>-    // changes to not call the virtual GetClassIDNoAlloc.
>-    *aClassID = (nsCID*) nsMemory::Alloc(sizeof(nsCID));
>-    if (!*aClassID)
>-        return NS_ERROR_OUT_OF_MEMORY;
>-    return GetClassIDNoAlloc(*aClassID);
>-}
>-
>-NS_IMETHODIMP 
>-nsJSURI::GetImplementationLanguage(PRUint32 *aImplementationLanguage)
>-{
>-    *aImplementationLanguage = nsIProgrammingLanguage::CPLUSPLUS;
>-    return NS_OK;
>-}
>-
>-NS_IMETHODIMP 
>-nsJSURI::GetFlags(PRUint32 *aFlags)
>-{
>-    *aFlags = nsIClassInfo::MAIN_THREAD_ONLY;
>-    return NS_OK;
>-}
> 
> NS_IMETHODIMP 
> nsJSURI::GetClassIDNoAlloc(nsCID *aClassIDNoAlloc)
> {
>     *aClassIDNoAlloc = kJSURICID;
>     return NS_OK;
>-}
>+} 

Again

>diff --git a/dom/src/jsurl/nsJSProtocolHandler.h b/dom/src/jsurl/nsJSProtocolHandler.h
>--- a/dom/src/jsurl/nsJSProtocolHandler.h
>+++ b/dom/src/jsurl/nsJSProtocolHandler.h
>@@ -14,17 +14,17 @@
>  *
>  * The Original Code is mozilla.org code.
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 1998
>  * the Initial Developer. All Rights Reserved.
>  *
>- * Contributor(s):
>+ * Contributor(s): Emanuele Costa <[e-mail]>

On a new line, with the first letter of your name aligned with the "n".

>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either of the GNU General Public License Version 2 or later (the "GPL"),
>  * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -39,16 +39,17 @@
> #define nsJSProtocolHandler_h___
> 
> #include "nsIProtocolHandler.h"
> #include "nsITextToSubURI.h"
> #include "nsIURI.h"
> #include "nsIMutable.h"
> #include "nsISerializable.h"
> #include "nsIClassInfo.h"
>+#include "nsSimpleURI.h"
> 
> #define NS_JSPROTOCOLHANDLER_CID                     \
> { /* bfc310d2-38a0-11d3-8cd3-0060b0fc14a3 */         \
>     0xbfc310d2,                                      \
>     0x38a0,                                          \
>     0x11d3,                                          \
>     {0x8c, 0xd3, 0x00, 0x60, 0xb0, 0xfc, 0x14, 0xa3} \
> }
>@@ -85,63 +86,45 @@ public:
> protected:
> 
>     nsresult EnsureUTF8Spec(const nsAFlatCString &aSpec, const char *aCharset, 
>                             nsACString &aUTF8Spec);
> 
>     nsCOMPtr<nsITextToSubURI>  mTextToSubURI;
> };
> 
>-// Use an extra base object to avoid having to manually retype all the
>-// nsIURI methods.  I wish we could just inherit from nsSimpleURI instead.
>-class nsJSURI_base : public nsIURI,
>-                     public nsIMutable
>+class nsJSURI : public nsSimpleURI
> {
> public:
>-    nsJSURI_base(nsIURI* aSimpleURI) :
>-        mSimpleURI(aSimpleURI)
>+
>+    nsJSURI() {}
>+
>+    nsJSURI(nsIURI* aBaseURI) : mBaseURI(aBaseURI) {}
>+    
>+    nsIURI* GetBaseURI() const 
>     {
>-        mMutable = do_QueryInterface(mSimpleURI);
>-        NS_ASSERTION(aSimpleURI && mMutable, "This isn't going to work out");
>-    }
>-    virtual ~nsJSURI_base() {}
>-
>-    // For use only from deserialization
>-    nsJSURI_base() {}
>-    
>-    NS_FORWARD_NSIURI(mSimpleURI->)
>-    NS_FORWARD_NSIMUTABLE(mMutable->)
>-
>-protected:
>-    nsCOMPtr<nsIURI> mSimpleURI;
>-    nsCOMPtr<nsIMutable> mMutable;
>-};
>-
>-class nsJSURI : public nsJSURI_base,
>-                public nsISerializable,
>-                public nsIClassInfo
>-{
>-public:
>-    nsJSURI(nsIURI* aBaseURI, nsIURI* aSimpleURI) :
>-        nsJSURI_base(aSimpleURI), mBaseURI(aBaseURI)
>-    {}
>-    virtual ~nsJSURI() {}
>-
>-    // For use only from deserialization
>-    nsJSURI() : nsJSURI_base() {}
>-
>-    NS_DECL_ISUPPORTS
>-    NS_DECL_NSISERIALIZABLE
>-    NS_DECL_NSICLASSINFO
>-
>-    // Override Clone() and Equals()
>-    NS_IMETHOD Clone(nsIURI** aClone);
>-    NS_IMETHOD Equals(nsIURI* aOther, PRBool *aResult);
>-
>-    nsIURI* GetBaseURI() const {
>         return mBaseURI;
>     }
> 
>+    NS_DECL_ISUPPORTS_INHERITED
>+
>+    // nsIURI overrides
>+    NS_IMETHOD Equals(nsIURI* other, PRBool *result);
>+    virtual nsSimpleURI* StartClone();
>+
>+    // nsISerializable overrides
>+    NS_IMETHOD Read(nsIObjectInputStream* aStream);
>+    NS_IMETHOD Write(nsIObjectOutputStream* aStream);
>+
>+    // Override the nsIClassInfo method GetClassIDNoAlloc to make sure our
>+    // nsISerializable impl works right.
>+    NS_IMETHOD GetClassIDNoAlloc(nsCID *aClassIDNoAlloc);  
>+
> private:
>     nsCOMPtr<nsIURI> mBaseURI;
> };
>+
>+
>+
>+
>+

Lose these lines.

>     
> #endif /* nsJSProtocolHandler_h___ */

Thanks for your patch!

Comment 6

[Emanuele Costa]

Attachment: Revised patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 525944 [details] [diff] [review]
Revised patch

>Now dom/src/jsurl/nsProtocolHandler inherits directly from nsSimpleURI. Few files need to be changed including two Makefiles.

How about:

  Make nsJSURI inherit from nsSimpleURI.  Bug 647570, r=bzbarsky

or something along those lines?

>+++ b/dom/src/jsurl/nsJSProtocolHandler.cpp
>@@ -193,29 +194,29 @@ nsresult nsJSThunk::EvaluateScript(nsICh
>-		PRBool allowsInline;
>-		rv = csp->GetAllowsInlineScript(&allowsInline);
>-		NS_ENSURE_SUCCESS(rv, rv);
>+    PRBool allowsInline;
>+    rv = csp->GetAllowsInlineScript(&allowsInline);
>+    NS_ENSURE_SUCCESS(rv, rv);

(and more in this method)

Please don't make those whitespace changes.

> NS_INTERFACE_MAP_BEGIN(nsJSURI)
...
>-      foundInterface = static_cast<nsIURI*>(this);
>+     foundInterface = static_cast<nsIURI*>(this);

Again, please leave the whitespace as it was.

> nsJSURI::Read(nsIObjectInputStream* aStream)
>+    rv = aStream->ReadObject(PR_TRUE, getter_AddRefs(mBaseURI));

You really need that check for the "have base URI" boolean the code used to have.

>+nsJSURI::Write(nsIObjectOutputStream* aStream)
>+{ 
>+  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(mBaseURI);
>+    if (!serializable) {

No need to worry about this.  If it's not, then WriteCompoundObject will fail, and things will be fine.

In particular, this change breaks ability to serialize when mBaseURI is null, which seems undesirable.

>+nsJSURI::Equals(nsIURI* other, PRBool *result)

Probably better to leave this after StartClone like it was before.

>+    *result = PR_FALSE;
>+
>+    if(!*result)

That will always test true, why bother with it?

>+        nsSimpleURI::Equals(other, result);
>+        if(PR_FALSE == *result)

This should handle nsSimpleURI::Equals returning an error.... and no need to compare a boolean to PR_FALSE explicitly.

>+        nsRefPtr<nsJSURI> jsURI;
>+        nsresult rv = other->QueryInterface(kJSURICID,
>+                                       getter_AddRefs(jsURI));
>+        NS_ENSURE_SUCCESS(rv, rv);

On the other hand, here we should just return NS_OK and *aResult == false if the QI fails.

>+        if(mBaseURI)

Space after "if", please.

>+nsJSURI::StartClone()
>+    NS_ENSURE_TRUE(mBaseURI, nsnull);

Why should cloning with null mBaseURI fail?

The rest looks good.

Comment 8

[Emanuele Costa]

Attachment: revised patch nsJSProtocolHandler

Fixed few things including the StartClone logic. Also whitespaces should not there. All mochitests pass.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 526196 [details] [diff] [review]
revised patch nsJSProtocolHandler

> Also whitespaces should not there.

There are still several places where you're changing 4-space indent to 3-space indent (e.g. in QueryInterface, in Read).

Please don't change the last line of Read: it's returning NS_OK instead of rv for good reasons.

You added a trailing space after the '{' at the beginning of Write's body.  Please take it out.  You also don't need to QI mBaseURI to nsISerializable in Write(), but _do_ need to keep writing the things we used to write.  This part is just broken right now: the Read doesn't read the things Write wrote.

You still need the space after "if" in Equals I commented about before.

I'm going to be offline until Wednesday, most likely, so the next review will be a bit slower.

Comment 10

[Emanuele Costa]

Attachment: nsJSProtocolHandler patch

I did you few things. Set up my emacs for trailing spaces and removed them. Please notice there were some trailing spaces also in the old code and I removed them as well if you don't mind.

"If statements" now follows the standard with a space after.

I decided to re-implement write using the old logic with the nsnull check on mBaseURI and returning NS_OK as well. So now the same things should be read/written.

I will start now working on the xpcshell test (but I need to quickly learn js and I will be full time on this only after my exam on the 3rd of May). 

In the meantime please let me know if the patch is ok and if I am on the right track with trailing spaces, 4 char indents, read/write logic and so on. 

All mochitests passes (also the top level ones in the dom root) and I tried to run the xpcshells tests in netwerk and chrome where nsJSProtocolHandler is also used and they are ok (but as discussed some functionalities may not be tested so I will work on the xpcshell test that is also an opportunity for me to learn js).

Comment 11

[Emanuele Costa]

Attachment: nsJSProtocolHandler patch

Sorry wrong file sent, see previous comments.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I removed them as well if you don't mind.

Actually I sort of do.  It's better to not randomly mess with whitespace.  If it needs fixing up, that should be done in a separate patch with no functional changes in it...  Can you please revert the whitespace changes?

Comment 13

[Emanuele Costa]

Attachment: nsJSProtocolHandler patch

removed trailing whitespace (reverted to original)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 527934 [details] [diff] [review]
nsJSProtocolHandler patch

r=me.  This looks great!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Emanuele, let me know whether you're still working on more tests or whether you want me to check this in as-is, ok?

Comment 16

[Emanuele Costa]

Boris, it is ok to check-in, when I am back on the 4th I will look for the other classes that need to inherit from nsSimpleURI (if any) and want to discuss with you some additional xpcshell tests (the current class is fully tested by mochitests anyway, in different part of the system like netwerk and chrome)

Comment 17

[Emanuele Costa]

and of course by mochitests in dom/src/jsurl (although some tests can be added there as I want to discuss)

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This landed as http://hg.mozilla.org/mozilla-central/rev/ea9f9f5503fa, but was backed out in http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=07de8acf5d7a because it was failing a test.

Emanuele, let Boris or me know if you need help figuring out how to run the particular test that was failing and getting started debugging it.

Comment 19

[Daniel Holbert [:dholbert]]

The failing tests, for reference, are:
 - editor/libeditor/html/tests/test_bug611182.html
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1303943741.1303945457.30360.gz

 - layout/reftest/tests/layout/reftests/bugs/572598-1.html and
   layout/reftests/editor/caret_on_presshell_reinit-2.html
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1303943774.1303945396.30221.gz

 - xpcshell/tests/dom/tests/unit/test_bug465752.js
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1303943779.1303945133.29088.gz

(the above tests all appear to have failed on all platforms, except for caret_on_presshell_reinit-2.html, which failed on mac/linux but which passed on the one windows reftest run that's completed so far.)

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The last one is the only relevant one to this bug.  The others are other problems with the same push.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yeah, so the problem is the xpcshell test at dom/tests/unit/test_bug465752.js

The issue is here:

  do_check_false(simple.equals(uri));

This now tests true, because |simple| thinks |uri| is just an nsSimpleURI.

The right way to fix this is probably to factor out the string equality tests from nsSimpleURI::Equals into a helper method, make nsJSURI _not_ QI to the simple URI CID, and change nsJSURI::Equals to check that the other QIs to nsJSURI and if so call the helper method that checks the string stuff.

Comment 22

[Emanuele Costa]

Ok, will work on this bug following Boris suggestions.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Hmm, it seems kind of bogus to me that nsIURI::Equals is expected to return false for two objects made from the same text just because they have different underlying C++ implementations ...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Well, two URIs should return false for equals() if dereferencing them will do different things, no?

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I suppose so.  In reality you'd never get in this situation where you have URIs with identical text content but different impls if you were asking the IO service for your URI (and had a sane protocol handler), right?

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I ask because nsFileDataURI almost certainly has the same bug, if this is in fact a bug.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> if you were asking the IO service for your URI

Yes, then there would be no problem.  But we have a history of UI+extension code written by people who don't know what they're doing creating URIs by hand.

Maybe we should make it easier to do the right thing here somehow for simple uri subclasse....

Comment 28

[Emanuele Costa]

Hi guys, the issue seems to be different. The dom/tests/unit/test_bug465752.js
 is actually failing at:

do_check_eq(simple.spec, uri.spec);

I wrote my first xpcshell test and verified it separately, the rest is ok including the check_false for different impls like 

do_check_false(simple.equals(uri));

I assume the spec should indeed be equal since they are the str url. Is that correct?

Comment 29

[Emanuele Costa]

and the reason of the failure being quite interesting:

*** uri spec http://www.example.com/
*** simple spec http://www.example.com

so uri (Components.interfaces.nsIIOService) adds a trailing "/" while Components.interfaces.nsIURI leaves the str unchanged. 

Testing code below:

function run_test()
{
const ios = Components.classes["@mozilla.org/network/io-service;1"]
                        .getService(Components.interfaces.nsIIOService);

const str = "http://www.example.com";
var uri = ios.newURI(str, null, null);


var simple = Components.classes["@mozilla.org/network/simple-uri;1"]
                         .createInstance(Components.interfaces.nsIURI);

simple.spec = str;
print("*** uri spec " + uri.spec);
print("*** simple spec " + simple.spec);
do_check_eq(simple.spec, uri.spec);
}

which implementation is correct? or they are meant to differ like they do?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> *** uri spec http://www.example.com/
> *** simple spec http://www.example.com

Uh... how could that possibly happen?  simple.spec is assigned "javascript:10".  There should be no example.com anywhere there.

Your modified test is completely different from the original test; both behaviors there are correct and the do_check_eq is asserting something that's just false in that situation.

Comment 31

[Emanuele Costa]

yes I see. if I put javascript:10 then no modification happens (trailing slash) and test fails where it is expected to fail. So I guess the trailing slash is added to URL. 

Ok, then if same string but different underlying implementations should still return false, is that correct? but it is a bit confusing.

Comment 32

[Boris Zbarsky [:bzbarsky]]

I think same string but different underlying implementation should return false, yes.

Comment 33

[Emanuele Costa]

Attachment: nsJSProtocolHandler patch

This is passing all tests (mochitest and unit, dom and netwerk) and implements the logic discussed with Boris. The class should complete now.

Comment 34

[Emanuele Costa]

Attachment: nsJSProtocolHandler patch

Hi Boris, I have synch the patches with the latest changes in nsSimpleURI. Please have a look. I kept the string check in the EqualsInternal separate to solve the inheritance problem and for performance reasons.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534713 [details] [diff] [review]
nsJSProtocolHandler patch

>+++ b/dom/src/jsurl/Makefile.in
>+

Lose that blank line.

>+++ b/dom/src/jsurl/nsJSProtocolHandler.cpp
> NS_INTERFACE_MAP_BEGIN(nsJSURI)
...
>+   else if(aIID.Equals(kThisSimpleURIImplementationCID))
>+   {
>+       foundInterface = nsnull;

You actually want *aInstancePtr = nsnull here.

Also, file style is to have the opening curly on the same line as the if, and to have a space after "if".  That applies in a few places here.

>+nsJSURI::StartClone()

This is not merged correctly with the nsSimpleURI ref changes.

>+      rv = nsSimpleURI::EqualsInternal(otherJSURI->mScheme, otherJSURI->mPath, result);

I think this EqualsInternal method should just take an nsSimpleURI* and be nonvirtual.  And return a bool; no need for COM here.

It should also be shared by the existing nsSimpleURI::Equals so we don't duplicate code.

That will incidentally make things work correctly for ref stuff.

I've gone ahead and made the above changes; will push this to cedar in a bit.

Comment 36

[Daniel Holbert [:dholbert]]

bz pushed this to cedar as: http://hg.mozilla.org/projects/cedar/rev/8641afbd20e6

Comment 37

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/8641afbd20e6

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 534713 [details] [diff] [review]
nsJSProtocolHandler patch

Requesting approval to land on aurora, because this is code-cleanup that bug 659698 layers on top of, and I'd like to get bug 659698 into aurora.

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 534713 [details] [diff] [review]
nsJSProtocolHandler patch

Canceling approval request, after checking with bz in IRC.  I can write a version of bug 659698 that doesn't layer on top of this.