1442239 - Disable nsISerializable.Read from nsIURI implementations

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

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

nsISerializable.Read can be used to mutate the URI objects, so we need to make them return an error code, and make deserialization happen via a mutator.

Comment 1

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

Attachment: Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

* Deserialization happens via a mutator
* The CID for URI implementations is translated to the associated mutator in nsBinaryStream

Review commit: https://reviewboard.mozilla.org/r/224284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224284/

Comment 2

[Masatoshi Kimura [:emk]]

Doesn't it break session restore?

Comment 3

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

(In reply to Masatoshi Kimura [:emk] from comment #2)
> Doesn't it break session restore?

No. The serialization/deserialization still happens in the same way, but is only accessible through the nsIURIMutator.

Comment 4

[Honza Bambas (:mayhemer)]

I'm not happy we have to, on every object being deserialized, search a mapping of a cid should it be one of our many URI objects.  Also, when we create a new URI, we have to remember to update that method, I don't like that even more.  I think we can do an exception and allow Read() to mutate a URI.

where possible I would do a check at respective Read() implementations that the URI is "brand new" - e.g. when spec is empty or so, and throw if it doesn't pass, leaving a URI readable only once.

Comment 5

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

That's a fair point. What do you think of this approach: We get nsIURIMutator to implement nsISerializable (we only care about Read); we return the mutator for each URI's CID... and we get QueryInterface to Finalize the mutator if the iid is for one of the interfaces implemented by the mURI it holds.
It may even be a little cleaner that way.

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1d8c0475dca98e1eb2c3c5df6c4ece4d975c9e0

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #5)
> That's a fair point. What do you think of this approach: We get
> nsIURIMutator to implement nsISerializable (we only care about Read); we
> return the mutator for each URI's CID... and we get QueryInterface to
> Finalize the mutator if the iid is for one of the interfaces implemented by
> the mURI it holds.
> It may even be a little cleaner that way.

I don't know why something smells bad on this to me...  I'd rather leave the nsISerializable on URIs and let them be somewhat mutable through it.  Can't we really make Read() fail when the URI is non-empty?

Comment 8

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

(In reply to Honza Bambas (:mayhemer) from comment #7)
> (In reply to Valentin Gosu [:valentin] from comment #5)
> > That's a fair point. What do you think of this approach: We get
> > nsIURIMutator to implement nsISerializable (we only care about Read); we
> > return the mutator for each URI's CID... and we get QueryInterface to
> > Finalize the mutator if the iid is for one of the interfaces implemented by
> > the mURI it holds.
> > It may even be a little cleaner that way.
> 
> I don't know why something smells bad on this to me...  I'd rather leave the
> nsISerializable on URIs and let them be somewhat mutable through it.  Can't
> we really make Read() fail when the URI is non-empty?

My main problem isn't that URIs would still be a little mutable. It's that I would like to make all the constructors of nsIURI implementations private. I'll put up the patch, and if you don't like it, we can leave this as a follow up, and maybe think of a better way of doing it :)

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #8)
> I
> would like to make all the constructors of nsIURI implementations private.

that was something I wanted to ask about too!  why should we still be able to xpcom-construct URIs?  

what if a URI would not impl nsISerializable at all and we would only allow to serialize via a specific mutator?  just throwing thoughts now...

> I'll put up the patch, and if you don't like it, we can leave this as a
> follow up, and maybe think of a better way of doing it :)

the patch on try?  I was looking at it.  that's the one that smells funny to me :)

Comment 10

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

(In reply to Honza Bambas (:mayhemer) from comment #9)
> (In reply to Valentin Gosu [:valentin] from comment #8)
> > I
> > would like to make all the constructors of nsIURI implementations private.
> 
> that was something I wanted to ask about too!  why should we still be able
> to xpcom-construct URIs?  
> 
> what if a URI would not impl nsISerializable at all and we would only allow
> to serialize via a specific mutator?  just throwing thoughts now...

I think they still need to implement nsISerializable so we can serialize them into session restore. Since serializing a URI doesn't mutate it in any way, I don't know why we'd change it.
Maybe I don't understand what you mean... but if we want to still be able to session restore from old profiles, I don't see a lot of ways of doing it :)

> > I'll put up the patch, and if you don't like it, we can leave this as a
> > follow up, and maybe think of a better way of doing it :)
> 
> the patch on try?  I was looking at it.  that's the one that smells funny to
> me :)

I see. Any specific issues with the patch, or are you worried that the QI hackery will cause problems in the future?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #10)
> Maybe I don't understand what you mean... 

just ignore that proposal (too late time for me...)

> 
> I see. Any specific issues with the patch, or are you worried that the QI
> hackery will cause problems in the future?

not really, it's just my guts don't like it.  but in general I think it's fine to do it this way.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8955126 [details]
Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

https://reviewboard.mozilla.org/r/224284/#review230282

so, can you please file and link followups for this?  as I understood from some of your comments, the next generation should remove cid/contracts for uris completely and make them constructable via the mutator only.  not sure how those should serialize then, but I assume that you are about to remove the mutator-specific cids added in this patch and make the now URI specific cids create mutators instead.  preserves compat and is slick :)

::: netwerk/base/nsIURIMutator.idl:279
(Diff revision 1)
>    nsIURI finalize();
>  };
>  
>  %{C++
>  
> +MOZ_MUST_USE bool ToMutatorCIDorSame(nsCID* aCID);

comments

Comment 13

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

Comment on attachment 8955126 [details]
Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224284/diff/1-2/

Comment 14

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

Comment on attachment 8955126 [details]
Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

So, I don't want to push the previous path at all for 2 reasons:
1. ToMutatorCIDorSame would actually make deserialization slower for non-URI objects
2. We are in the soft-freeze period and the change did seem a bit risky

This new approach seems a bit better.
I moved all the QueryInterface hackery into a macro, so it's less verbose.
This approach should have no performance overhead.

I hope to land this and next patches very soon after the merge day, so we can benefit from thread-safe URIs as much as possible in the next release.

Comment 15

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

Attachment: Bug 1442239 - Fix test since nsSimpleURI can't be constructed via createInstance() anymore

* I am not entirely sure what this test is doing, but I found that replacing nsSimpleURI with CSPContext makes it work.

Review commit: https://reviewboard.mozilla.org/r/226072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226072/

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 8957137 [details]
Bug 1442239 - Fix test since nsSimpleURI can't be constructed via createInstance() anymore

https://reviewboard.mozilla.org/r/226072/#review232948

Why are you asking me to review this?  It's not like I wr...oh.

Comment 17

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

Comment on attachment 8955126 [details]
Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

I think the updated patch still needs a review.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 8955126 [details]
Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

https://reviewboard.mozilla.org/r/224284/#review234660

::: netwerk/build/nsNetModule.cpp
(Diff revision 2)
>      { NS_UDPSOCKET_CONTRACTID, &kNS_UDPSOCKET_CID },
>      { NS_SOCKETPROVIDERSERVICE_CONTRACTID, &kNS_SOCKETPROVIDERSERVICE_CID },
>      { NS_DNSSERVICE_CONTRACTID, &kNS_DNSSERVICE_CID },
>      { NS_IDNSERVICE_CONTRACTID, &kNS_IDNSERVICE_CID },
>      { NS_EFFECTIVETLDSERVICE_CONTRACTID, &kNS_EFFECTIVETLDSERVICE_CID },
> -    { NS_SIMPLEURI_CONTRACTID, &kNS_SIMPLEURI_CID },

should we also remove NS_*URI_CONTRACTID definitions when they are no longer used?

Comment 19

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

Comment on attachment 8955126 [details]
Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

https://reviewboard.mozilla.org/r/224284/#review234660

> should we also remove NS_*URI_CONTRACTID definitions when they are no longer used?

I removed it in bug 1442242

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aef4c73f736f
Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/513cd669aca1
Fix test since nsSimpleURI can't be constructed via createInstance() anymore r=Waldo

Comment 21

[Andreea Pavel [:apavel]]

Backed out for xpcshell failures at widget/tests/unit/test_taskbar_jumplistitems.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=513cd669aca140b8792069c05c4b5215d88bb41a

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168993666&repo=autoland&lineNumber=10150

Backout: https://hg.mozilla.org/integration/autoland/rev/49d1cb161b40922d872836674ec92d0b7a8aa7b0

Comment 22

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

Comment on attachment 8955126 [details]
Bug 1442239 - Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224284/diff/2-3/

Comment 23

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

Comment on attachment 8957137 [details]
Bug 1442239 - Fix test since nsSimpleURI can't be constructed via createInstance() anymore

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226072/diff/1-2/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61aa0247279a
Make URI deserialization (nsISerializable.read) happen via nsIURIMutator only r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/377b62a130e7
Fix test since nsSimpleURI can't be constructed via createInstance() anymore r=Waldo

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/61aa0247279a
https://hg.mozilla.org/mozilla-central/rev/377b62a130e7