1416791 - Reduce the mutability of nsHostObjectURI

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Description

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

Making nsHostObjectURI immutable requires that we get rid of the current mutability, or move it out of the object.
This means ForgetBlobImpl and extending nsSupportsWeakReference should be removed or moved into nsHostObjectProtocolHandler.

Comment 1

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

Attachment: Reduce the mutability of nsHostObjectURI

- Makes nsHostObjectURI not extend nsSupportsWeakReference
- Tracks blobImpls via nsRefPtrHashtable<nsPtrHashKey<nsIURIWithBlobImpl>, BlobImpl> gBlobTable
- GetBlobImpl and ForgetBlobImpl use the hashtable instead of a member variable

MozReview-Commit-ID: AjLsXMTt1np

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8927869 [details] [diff] [review]
Reduce the mutability of nsHostObjectURI

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

I'm not totally sure I like the 2 tables. Maybe we can merge them and mark DataInfo to be revoked while we are waiting for the timer to delete the data for real.
If we do this, nsHostObjectURI just needs to have a immutable blob URL as string in order to retrieve BlobImpl from gDataTable.
If you do this, you can extend GetDataInfo() to be:

static DataInfo*                                                                                          
GetDataInfo(const nsACString& aUri, bool aConsiderReleasedObject = false)

Also moving nsHostObjectURI code in nsHostObjectProtocolHandler would be nice if it can be avoid.

Comment 3

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

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

Comment 4

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

(In reply to Andrea Marchesini [:baku] from comment #2)
> I'm not totally sure I like the 2 tables. Maybe we can merge them and mark
> DataInfo to be revoked while we are waiting for the timer to delete the data
> for real.
> If we do this, nsHostObjectURI just needs to have a immutable blob URL as
> string in order to retrieve BlobImpl from gDataTable.

I have an implementation of this. It makes one relevant change from previous behaviour:
If before we called nsHostObjectProtocolHandler::NewURI(spec), and the blob for spec had just been revoked, then the URI would have a null blobImpl. With this change, it would still be able to find the blobImpl in the table until it's removed by the timer.

Is this OK?

Comment 5

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

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

Comment 6

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

Attachment: Bug 1416791 - Eliminate mutability from nsHostObjectURI by removing nsIURIWithBlobImpl

* blobImpl references are now only kept in nsHostObjectProtocolHandler
* removes nsHostObjectProtocolHandler.idl
* Makes nsHostObjectURI no longer inherit from nsSupportsWeakReference

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

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8957602 [details]
Bug 1416791 - Eliminate mutability from nsHostObjectURI by removing nsIURIWithBlobImpl

https://reviewboard.mozilla.org/r/226486/#review232588

::: dom/file/nsHostObjectURI.h:31
(Diff revision 1)
>    , public nsIURIWithPrincipal
> -  , public nsIURIWithBlobImpl
> -  , public nsSupportsWeakReference
>  {
>  private:
> -  nsHostObjectURI(nsIPrincipal* aPrincipal,
> +  nsHostObjectURI(nsIPrincipal* aPrincipal)

explicit

Comment 8

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

Comment on attachment 8957602 [details]
Bug 1416791 - Eliminate mutability from nsHostObjectURI by removing nsIURIWithBlobImpl

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aff4d536de14
Eliminate mutability from nsHostObjectURI by removing nsIURIWithBlobImpl r=baku

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/aff4d536de14