1447272 - Port Bug 1442239 - Disable nsISerializable.Read from nsIURI implementations

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Looks like we need to remove the nsXXXUrl::Mutator::Read() methods, example here:
https://hg.mozilla.org/mozilla-central/rev/61aa0247279a#l6.12

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-remove-mutator-read.patch

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3ab4909ae468
Port Bug 1442239 to mailnews: Remove nsXXXUrl::Mutator::Read() methods. rs=bustage-fix

Comment 3

[Jorg K (CEST = GMT+2)]

Looks like this was the wrong fix or at least an incomplete fix. TB crashes pretty much straight away and all tests are red (due to crashes). Looks like it crashes when reading parts for the URL, like in GetScheme() or IsScheme().

The M-C changes https://hg.mozilla.org/mozilla-central/rev/61aa0247279a don't give a clear example.

Comment 4

[Jorg K (CEST = GMT+2)]

Valentin, can you please advise.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

So, what this patch did was remove the ability to initialize a URL by calling ::Read() on it directly.
What you have to do instead, is create a mutator of the type you want, and call Read on that. The mutator will forward the call to ReadPrivate().

So what your fix should do:
For nsIURI implementations that also inherit nsISerializable you should move all the code from Read() to ReadPrivate(). Read should only contain:
>  NS_NOTREACHED("Use nsIURIMutator.read() instead");
>  return NS_ERROR_NOT_IMPLEMENTED;

Then the mutator for each of those classes should extend nsISerializable. The Write implementation should return NS_ERROR_NOT_IMPLEMENTED. The Read implementation should call InitFromInputStream(aStream)

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-adjust-read-write.patch

OK, the previous patch was completely wrong and will be backed out. This one compiles so far, let's see whether it runs.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-adjust-read-write.patch (v1b)

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-adjust-read-write.patch (v1c)

This does what you decribed, however, TB just doesn't work and crashes.

Before it crashes, it spits out a lot of errors, like:
[7532, Main Thread] WARNING: '!aURI', file c:/mozilla-source/comm-central/mozill
a/netwerk/base/nsIURIMutatorUtils.cpp, line 8
[7532, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(mStatus) failed with result
0x80070057: file c:/mozilla-source/comm-central/mozilla/netwerk/base/nsIURIMutat
orUtils.cpp, line 17
[7532, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, mStatus) failed with res
ult 0x80070057: file c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\i
nclude\nsIURIMutator.h, line 607
[7532, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8007
0057: file c:/mozilla-source/comm-central/mailnews/base/util/nsMsgMailNewsUrl.cp
p, line 421

nsMsgMailNewsUrl.cpp, line 421 is
  nsresult rv = NS_MutateURI(m_baseURL).SetSpec(aSpec).Finalize(m_baseURL);
  NS_ENSURE_SUCCESS(rv, rv);

Comment 9

[Valentin Gosu [:valentin] (he/him)]

nsIURIMutatorUtils.cpp, line 8
The problem here is that m_BaseURL is probably null.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8960670 [details] [diff] [review]
1447272-adjust-read-write.patch (v1c)

Review of attachment 8960670 [details] [diff] [review]:
-----------------------------------------------------------------

::: ldap/xpcom/src/nsLDAPURL.h
@@ +70,5 @@
>      }
>  
>      NS_IMETHOD Read(nsIObjectInputStream* aStream) override
>      {
> +      NS_NOTREACHED("Use nsIURIMutator.read() instead");

this should just be:

return InitFromInputStream(aStream);

@@ +76,5 @@
>      }
>  
> +    nsresult ReadPrivate(nsIObjectInputStream* aStream)
> +    {
> +      return InitFromInputStream(aStream);

ReadPrivate must be defined on the URI, not the mutator. InitFromInputStream is the one calling ReadPrivate.

::: mailnews/addrbook/src/nsAddbookUrl.h
@@ +62,5 @@
> +    }
> +
> +    NS_IMETHOD Write(nsIObjectOutputStream *aOutputStream) override
> +    {
> +        return NS_ERROR_NOT_IMPLEMENTED;

same for this mutator.

::: mailnews/compose/src/nsSmtpUrl.h
@@ +100,5 @@
>      eHonorRef,
>      eReplaceRef
>    };
>    virtual ~nsMailtoUrl();
> +  nsresult ReadPrivate(nsIObjectInputStream *aStream);

So, you should actually define nsMailtoUrl::ReadPrivate() not just declare it.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Valentin Gosu [:valentin] from comment #10)
> So, you should actually define nsMailtoUrl::ReadPrivate() not just declare
> it.
OK, and what should it do? Sorry about the (silly/ignorant) question.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) from comment #11)
> (In reply to Valentin Gosu [:valentin] from comment #10)
> > So, you should actually define nsMailtoUrl::ReadPrivate() not just declare
> > it.
> OK, and what should it do? Sorry about the (silly/ignorant) question.

URIImplementation::ReadPrivate should do exactly what Read does now.
URIImplementation::Read should call NS_NOTREACHED so we don't call it by mistake, and return not_implemented.
URIImplementation::Mutator::Read should call initFromStream
URIImplementation::Mutator::Write should return not implemented.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Valentin Gosu [:valentin] from comment #12)
> URIImplementation::ReadPrivate should do exactly what Read does now.
Yes, that's what I saw in your patches, for example here:
https://hg.mozilla.org/mozilla-central/rev/61aa0247279a#l12.12

The only problem is, our URL's never had a Read() method. And the mutator Read() said "not implemented". So now the Mutator::Read calls initFromStream() which in turn calls ReadPrivate() on the URL which we don't have and never had.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) from comment #13)
> (In reply to Valentin Gosu [:valentin] from comment #12)
> > URIImplementation::ReadPrivate should do exactly what Read does now.
> Yes, that's what I saw in your patches, for example here:
> https://hg.mozilla.org/mozilla-central/rev/61aa0247279a#l12.12
> 
> The only problem is, our URL's never had a Read() method. And the mutator
> Read() said "not implemented". So now the Mutator::Read calls
> initFromStream() which in turn calls ReadPrivate() on the URL which we don't
> have and never had.

Oh, if the URI doesn't implement nsISerializable (so it doesn't have Read/Write methods) - then the mutator for that class shouldn't implement nsISerializable.

Comment 15

[Jorg K (CEST = GMT+2)]

Right, that's why the patch I landed
https://hg.mozilla.org/comm-central/rev/3ab4909ae468
Port Bug 1442239 to mailnews: Remove nsXXXUrl::Mutator::Read() methods. rs=bustage-fix
removed the Read() methods from the mutator. That compiled, but it crashes left, right and centre, and about all our tests fail, see comment #3.

So where from here?

Comment 16

[Valentin Gosu [:valentin] (he/him)]

In that case we probably need to audit all the classes that implement nsIURI and nsISerializable, or that extend a class that does.
I assume there are a few.

Comment 17

[Jorg K (CEST = GMT+2)]

Hmm, I'm not sure I follow. We have four URL classes: address book, LDAP, MailTo. These extend nsIURI, that is, the objects have a pointer to a "base URL" of nsIURI. The 4th class, the "mailnews URL" extends nsIURL and has a pointer to nsIURL. None of these classes have a Read() method so far. And I had never heard of nsISerializable, it's not used in C-C.

Comment 18

[Valentin Gosu [:valentin] (he/him)]

You're right, looking on searchfox, it seems comm-central URLs don't extend nsISerializable.
The initial patch removing the Read methods should have worked.
The build and test failures don't make much sense to me. As I said in comment 9, the error there is that the URL being mutated is null.

Comment 19

[Jorg K (CEST = GMT+2)]

Hmm, if you're referring to the build failures you can see on the C-C tree, they have a different cause.

We all agree that the problem is that the URL being mutated is null.

I've just tracked one case:
nsImapService::CreateStartOfImapUrl()
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mailnews/imap/src/nsImapService.cpp#1345
That creates an "IMAP URL", QI's it to nsIMsgMailNewsUrl, then calls nsIMsgMailNewsUrl::SetSpecInternal() and that thing mutates the underlying URL (m_baseURL), which appears to be null.

That code worked until today, so I'm not sure what's changed.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8960529 [details] [diff] [review]
1447272-remove-mutator-read.patch

Maybe not so wrong after all ;-)

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-SetSpecInternal.patch

This avoids the crash, but I still get heaps of errors from
nsSmtpService.cpp line 318 and nsSmtpUrl.cpp line 269.

Comment 22

[Valentin Gosu [:valentin] (he/him)]

So, SetSpecInternal doesn't really need to mutate anything. You can just create a new mutator and SetSpec on that.

Comment 23

[Jorg K (CEST = GMT+2)]

See where this gets us:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ae3606097f3b95f8fa02d5ca3c8726fbe49f9dd0

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to Valentin Gosu [:valentin] from comment #22)
> So, SetSpecInternal doesn't really need to mutate anything. You can just
> create a new mutator and SetSpec on that.
I think it doesn't really matter whether it mutates the old one or creates a new one straight away.

If you look at the constructor of nsMsgMailNewsUrl, it does
  m_baseURL = do_CreateInstance(NS_STANDARDURL_CONTRACTID);
And, BTW, all out URLs initialise that member in the constructor.

So I'm at a complete loss to understand what has happened here. The bug we're porting is about removing some nsISerializable.Read. Now, our URLs don't inherit from that, so removing the Read() methods should just do the trick.

But somehow the behaviour of the underlying URLs seems to have changed completely.
What does
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mailnews/imap/src/nsImapService.cpp#1345
actually do?
rv = CallCreateInstance(kImapUrlCID, imapUrl);
no longer creates a full-blown URL with the underlying base class nsMsgMailNewsUrl properly initialised?

Looking at my try push most of the failing tests seem to relate to the "mail to" URLs which show the errors mentioned in comment #21.

The code is this:
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mailnews/compose/src/nsSmtpService.cpp#315
calling this:
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mailnews/compose/src/nsSmtpUrl.cpp#268
m_baseURL was intialised in the constructor, so how can it be null?

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-SetSpecInternal.patch

OK, this clears up test_mailtoURL.js which still failed on the try. Let's see what else works.

Comment 26

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) from comment #24)
> (In reply to Valentin Gosu [:valentin] from comment #22)
> > So, SetSpecInternal doesn't really need to mutate anything. You can just
> > create a new mutator and SetSpec on that.
> I think it doesn't really matter whether it mutates the old one or creates a
> new one straight away.
> 
> If you look at the constructor of nsMsgMailNewsUrl, it does
>   m_baseURL = do_CreateInstance(NS_STANDARDURL_CONTRACTID);

Oh, this is the problem.
The deserialization code now instantiates a mutator when passed the CID for standardURL in order to be backwards compatible.
Meaning the code here [1] will create a mutator for the cid of nsStandardURL. The call to read will create it in the mutator, then QueryInterface will act just like the finalizer.

[1] https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/xpcom/io/nsBinaryStream.cpp#974

Problem is that now do_CreateInstance(NS_STANDARDURL_CONTRACTID) and all others (simpleURI, etc) return a mutator instead, and unless it's initialized by calling SetSpec for example, QIing it to nsIURI will fail and m_baseURL will be null.

My recommendation is for SetSpecInternal/etc to always construct a new URI using the mutator. Bug 1442242 will make all the URI constructors private anyway.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-SetSpecInternal.patch (v3)

Doing the same trick with the address book URL fixes more tests.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1447272-SetSpecInternal.patch (v4)

OK, that does what was suggested.

I'll leave this open to look into the other uses.
How about
  rv = CallCreateInstance(kImapUrlCID, imapUrl);
Does that need to be replaced?

I guess LDAP is OK for now?
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/ldap/xpcom/src/nsLDAPURL.cpp#40

Comment 29

[Jorg K (CEST = GMT+2)]

And we have plenty more, for example here:
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mailnews/base/util/nsMsgUtils.cpp#185

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/481987fc6713
Create a new base URL in nsXXXUrl::SetSpecInternal(). rs=bustage-fix

Comment 31

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) from comment #28)
> Created attachment 8960798 [details] [diff] [review]
> 1447272-SetSpecInternal.patch (v4)
> 
> OK, that does what was suggested.
> 
> I'll leave this open to look into the other uses.
> How about
>   rv = CallCreateInstance(kImapUrlCID, imapUrl);
> Does that need to be replaced?

Well, no. It would be great for maintainability if all URL implementations followed the same pattern, of using the mutator, but since it wasn't serializable, and I didn't change the way it's instantiated, it's OK for now.

> I guess LDAP is OK for now?
> https://dxr.mozilla.org/comm-central/rev/
> a8eecfe6de793af00e98cb1488515199c5fb73fb/ldap/xpcom/src/nsLDAPURL.cpp#40

That is OK. The contractID for the mutator still works.

Comment 32

[:aceman]

Comment on attachment 8960529 [details] [diff] [review]
1447272-remove-mutator-read.patch

Review of attachment 8960529 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 33

[:aceman]

Comment on attachment 8960798 [details] [diff] [review]
1447272-SetSpecInternal.patch (v4)

Review of attachment 8960798 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not very happy about the window where m_baseURL is null between the constructor and the first SetSpecInternal() call, but if that is the right way now...

Comment 34

[Valentin Gosu [:valentin] (he/him)]

(In reply to :aceman from comment #33)
> Comment on attachment 8960798 [details] [diff] [review]
> 1447272-SetSpecInternal.patch (v4)
> 
> Review of attachment 8960798 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not very happy about the window where m_baseURL is null between the
> constructor and the first SetSpecInternal() call, but if that is the right
> way now...

uninitialized isn't much better than null, IMO. This is why I'm making constructors private, so you are forced to use a mutator, and without proper error checking you get back a null URI.

Comment 35

[Jorg K (CEST = GMT+2)]

We still have a crash in mailnews/jsaccount/test/unit/test_fooUrl.js, but the rest looks good so far.

Comment 36

[Jorg K (CEST = GMT+2)]

Hmm, bug 1442242 already landed and bounced back. Will that affect the stuff mentioned in comment #28 and #29, #29 being:
  nsCOMPtr<nsIImapUrl> imapUrl = do_CreateInstance(kImapUrlCID, &rv);
  nsCOMPtr<nsIMailboxUrl> mailboxUrl = do_CreateInstance(kCMailboxUrl, &rv);
and more?

Comment 37

[Jorg K (CEST = GMT+2)]

I guess we're done here then, I filed bug 1447492 for mailnews/jsaccount/test/unit/test_fooUrl.js is crashing.

Comment 38

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) from comment #36)
> Hmm, bug 1442242 already landed and bounced back. Will that affect the stuff
> mentioned in comment #28 and #29, #29 being:
>   nsCOMPtr<nsIImapUrl> imapUrl = do_CreateInstance(kImapUrlCID, &rv);
>   nsCOMPtr<nsIMailboxUrl> mailboxUrl = do_CreateInstance(kCMailboxUrl, &rv);
> and more?

No.

It only removes the constructors for gecko URI implementations.
I don't think any of the TB URIs extend them, so it should be OK.