1227296 - NS_ConvertUTF8toUTF16 and NS_ConvertUTF16toUTF8 do not preserve voidness

Status: NEW

(Core :: XPCOM, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

The IsVoid flag on a string should probably be preserved when converting across encodings.

I've written a failing test case which I will attach.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1227296 - Regression test.

Bug 1227296 - Regression test.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, when scc added the concept of Void to the string code to meet some DOM nullability requirements (that probably no longer exist with new bindings?!), he was tempted to call it "Purple" rather than "Void" to avoid people making assumptions about what it should do.  It was designed for the very limited usecase of DOM needing to support string-or-null return values.  Now that we have Nullable (dom/bindings/Nullable.h), we really shouldn't need that anymore.

I'm curious what people are using it for and whether we can remove those uses of Void on strings and then remove the concept completely.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #2)
> FWIW, when scc added the concept of Void to the string code to meet some DOM
> nullability requirements (that probably no longer exist with new
> bindings?!)
Paris bindings very much use void ns*Strings.

Looks like that is not too well documented in all the cases, but 
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#ByteString at least.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Ah, https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#DOMString does document that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, the basic problem is that we left the null domstring setup as-is in webidl bindings because we didn't want to audit all the callees at the time.

That said, we could definitely switch "DOMString?" return values to Nullable<nsString> or some such, and "DOMString?" in params to "const Nullable<nsAString>" and write those two specializations.

Apart from bindings code, the main consumers of this flag in Gecko seem to be places that are in fact trying to represent "string or null" for whatever reason.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8691005 [details]
MozReview Request: Bug 1227296 - Regression test.

https://reviewboard.mozilla.org/r/26045/#review23919

Always happy to r+ tests.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I'm not fully in tune with the conversation that occurred between comment 2 and comment 5... I think I'm missing some context. At any rate, is this still worth fixing?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I think yes

I haven't seen anyone filing a bug to convert all our void string usage to
Nullable<ns*String>. (That would be rather massive, yet mostly mechanical change.)