1447194 - Use threadsafe refcounting in nsIURI implementations

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

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

The last stage in making the now immutable implementations of nsIURI threadsafe is using threadsafe refcounting.

Comment 1

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

Attachment: Bug 1447194 - Use threadsafe refcounting in all nsIURI implementations

It seem that only nsStandardURL and nsSimpleURI (and classes that inherit them) do not have threadsafe refcounting yet.

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

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8960607 [details]
Bug 1447194 - Use threadsafe refcounting in all nsIURI implementations

https://reviewboard.mozilla.org/r/229368/#review235632

\   /
 \O/

are these two the only ones we can change this way?

Comment 3

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

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Comment on attachment 8960607 [details]
> Bug 1447194 - Use threadsafe refcounting in all nsIURI implementations
> 
> https://reviewboard.mozilla.org/r/229368/#review235632
> 
> \   /
>  \O/
> 
> are these two the only ones we can change this way?

Yes. The other implementations that don't extend standard/simpleURI are marked threadsafe (even though they weren't).
I suspect the fact that these were not threadsafe kept us from accessing nsIURIs from multiple threads.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/770603f0efe1
Use threadsafe refcounting in all nsIURI implementations r=mayhemer

Comment 5

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/770603f0efe1

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Can we uplift this to Beta to help clean up the MOZ_CrashOOL oranges we still see there since that branch is destined for ESR status?

Comment 7

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

I think we can. We would also have to uplift bug 1442239, bug 1442242, bug 1447190, bug 1447194 (this), bug 1447330 and bug 1453633. So far we haven't had any big issue on nightly. The only problem was bug 1450783 (slight performance regression).

Comment 8

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Can we uplift this to Beta to help clean up the MOZ_CrashOOL oranges we
> still see there since that branch is destined for ESR status?

Also, can you point out which MOZ_CrashOOL might be fixed by this? It would be nice to confirm if this would definitely fix them.

Comment 9

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

Comment on attachment 8960607 [details]
Bug 1447194 - Use threadsafe refcounting in all nsIURI implementations

Approval Request Comment
[Feature/Bug causing the regression]:
Not a regression.

[User impact if declined]:
It would be harder to uplift patches to the new ESR branch.
Also, see comment 6.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
bug 1442239, bug 1442242, bug 1447190, bug 1447194 (this), bug 1447330 and bug 1453633 - in this order.

[Is the change risky?]:
Low risk.

[Why is the change risky/not risky?]:
This patches have not surfaced any big issues on Nightly.
The only thing that would count as a regression is bug 1450783 - a slight slowdown caused by the atomic refcounting.

[String changes made/needed]:
None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Valentin Gosu [:valentin] from comment #8)
> Also, can you point out which MOZ_CrashOOL might be fixed by this? It would
> be nice to confirm if this would definitely fix them.

Bug 1445170 and its giant pile o' dupes claim to have been fixed by this. That said, I had no idea there were this many dependencies when I made that comment last night. This looks like a lot to uplift so late in the cycle :(

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8960607 [details]
Bug 1447194 - Use threadsafe refcounting in all nsIURI implementations

I don't think we should take this set of changes that late in beta.