1332568 - Make nsContentTypeParser use nsMIMEHeaderParamImpl::DoGetParameter directly

Status: RESOLVED INCOMPLETE

(Core :: Networking, defect, P5)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Currently nsContentTypeParser uses the nsIMIMEHeaderParam/"@mozilla.org/network/mime-hdrparam;1" service, meaning it can only be called on the main thread.
To make nsContentTypeParser thread-safe, it could use the (one and only) implementation directly, which is nsMIMEHeaderParamImpl.

(Is this controversial?)

My main reason for this is that I'd like to use the parser in other threads, e.g. when reading metadata asynchronously and extracting a MIME type.

It would also probably save a few cycles, as it won't have to fetch (and possibly create) the service every time it is needed.

Comment 1

[Patrick McManus [:mcmanus]]

sgtm. thanks

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for volunteering to review. :-)
(But please feel free to forward to someone else, as appropriate...)

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1332568 - Expose nsMIMEHeaderParamImpl.h and make DoGetParameter static and public -

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

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1332568 - Make nsContentTypeParser use nsMIMEHeaderParamImpl::DoGetParameter -

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

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1332568 - MediaMIMETypes can use thread-safe nsContentTypeParser -

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

Comment 6

[Honza Bambas (:mayhemer)]

Please fix the patches in rb or please reupload as normal patches so I can review them in splitner.  thanks.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8829325 [details]
Bug 1332568 - Expose nsMIMEHeaderParamImpl.h and make DoGetParameter static and public -

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

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8829326 [details]
Bug 1332568 - Make nsContentTypeParser use nsMIMEHeaderParamImpl::DoGetParameter -

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

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8829327 [details]
Bug 1332568 - MediaMIMETypes can use thread-safe nsContentTypeParser -

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

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: p1. Expose nsMIMEHeaderParamImpl.h and make DoGetParameter static and public

p1. Expose nsMIMEHeaderParamImpl.h and make DoGetParameter static and public

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: p2. Make nsContentTypeParser use nsMIMEHeaderParamImpl::DoGetParameter

p2. Make nsContentTypeParser use nsMIMEHeaderParamImpl::DoGetParameter

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: p3. MediaMIMETypes can use thread-safe nsContentTypeParser

p3. MediaMIMETypes can use thread-safe nsContentTypeParser

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8831823 [details] [diff] [review]
p1. Expose nsMIMEHeaderParamImpl.h and make DoGetParameter static and public

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

::: netwerk/mime/nsMIMEHeaderParamImpl.h
@@ +19,5 @@
>      MIME_FIELD_ENCODING = 1,
>      HTTP_FIELD_ENCODING
> +  };
> +  static
> +  nsresult DoGetParameter(const nsACString& aHeaderVal,

in the cpp file, please mark these two methods with 

// static 

comment, and please double check that none of them (including the callees) writes to any global memory

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8831824 [details] [diff] [review]
p2. Make nsContentTypeParser use nsMIMEHeaderParamImpl::DoGetParameter

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

::: dom/base/nsContentUtils.cpp
@@ +6134,5 @@
>  
>    return NS_SUCCEEDED(aPrincipal->CheckMayLoad(channelURI, false, aAllowIfInheritsPrincipal));
>  }
>  
>  nsContentTypeParser::nsContentTypeParser(const nsAString& aString)

please make sure that this class is not accessing any globals so it can be safely used on more than one thread.

Comment 15

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Abandonning this old bug I haven't touched for years! I don't think it's worth keeping around, patches are probably very bit-rotten, and if there was a need for this, someone else could probably do it better than me now, and open a bug for their own work.