Closed Bug 1442239 Opened 2 years ago Closed 2 years ago

Disable nsISerializable.Read from nsIURI implementations

Categories

(Core :: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files)

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.
Doesn't it break session restore?
(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.
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.
Flags: needinfo?(valentin.gosu)
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.
Flags: needinfo?(valentin.gosu)
(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?
(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 :)
(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 :)
(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?
(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 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
Attachment #8955126 - Flags: review?(honzab.moz) → review+
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.
Attachment #8955126 - Flags: review?(honzab.moz)
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.
Attachment #8957137 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8955126 - Flags: review?(honzab.moz)
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?
Attachment #8955126 - Flags: review?(honzab.moz) → review+
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
Pushed by valentin.gosu@gmail.com:
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
Pushed by valentin.gosu@gmail.com:
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
Flags: needinfo?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/61aa0247279a
https://hg.mozilla.org/mozilla-central/rev/377b62a130e7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1447330
You need to log in before you can comment on or make changes to this bug.