Closed Bug 1447272 Opened 2 years ago Closed 2 years ago

Port Bug 1442239 - Disable nsISerializable.Read from nsIURI implementations

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 61.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Looks like we need to remove the nsXXXUrl::Mutator::Read() methods, example here:
https://hg.mozilla.org/mozilla-central/rev/61aa0247279a#l6.12
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 61.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3ab4909ae468
Port Bug 1442239 to mailnews: Remove nsXXXUrl::Mutator::Read() methods. rs=bustage-fix
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Valentin, can you please advise.
Flags: needinfo?(valentin.gosu)
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)
Flags: needinfo?(valentin.gosu)
Attached patch 1447272-adjust-read-write.patch (obsolete) — Splinter Review
OK, the previous patch was completely wrong and will be backed out. This one compiles so far, let's see whether it runs.
Attachment #8960529 - Attachment is obsolete: true
Attachment #8960663 - Attachment is obsolete: true
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);
Attachment #8960665 - Attachment is obsolete: true
Attachment #8960670 - Flags: feedback?(valentin.gosu)
nsIURIMutatorUtils.cpp, line 8
The problem here is that m_BaseURL is probably null.
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.
Attachment #8960670 - Flags: feedback?(valentin.gosu)
(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.
(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.
(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.
(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.
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?
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.
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.
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.
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 on attachment 8960529 [details] [diff] [review]
1447272-remove-mutator-read.patch

Maybe not so wrong after all ;-)
Attachment #8960529 - Attachment is obsolete: false
Attached patch 1447272-SetSpecInternal.patch (obsolete) — Splinter Review
This avoids the crash, but I still get heaps of errors from
nsSmtpService.cpp line 318 and nsSmtpUrl.cpp line 269.
Attachment #8960670 - Attachment is obsolete: true
So, SetSpecInternal doesn't really need to mutate anything. You can just create a new mutator and SetSpec on that.
(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?
Flags: needinfo?(valentin.gosu)
Keywords: leave-open
Version: 60 → Trunk
Attached patch 1447272-SetSpecInternal.patch (obsolete) — Splinter Review
OK, this clears up test_mailtoURL.js which still failed on the try. Let's see what else works.
Attachment #8960733 - Attachment is obsolete: true
(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.
Doing the same trick with the address book URL fixes more tests.
Attachment #8960791 - Attachment is obsolete: true
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
Attachment #8960794 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/481987fc6713
Create a new base URL in nsXXXUrl::SetSpecInternal(). rs=bustage-fix
(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.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8960529 [details] [diff] [review]
1447272-remove-mutator-read.patch

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

Thanks.
Attachment #8960529 - Flags: review+
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...
Attachment #8960798 - Flags: review+
(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.
We still have a crash in mailnews/jsaccount/test/unit/test_fooUrl.js, but the rest looks good so far.
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?
I guess we're done here then, I filed bug 1447492 for mailnews/jsaccount/test/unit/test_fooUrl.js is crashing.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Keywords: leave-open
Resolution: --- → FIXED
(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.
Depends on: 1482720
You need to log in before you can comment on or make changes to this bug.