1410794 - Change some |string| occurrences in nsIPrefBranch.idl to |AUTF8String|

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

Because Gecko strings are nicer to work with than C strings.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Change some |string| occurrences in nsIPrefBranch.idl to |AUTF8String|

This makes the code nicer. In particular, it removes many getter_Copies()
calls. The patch also converts a lot of nsCStrings to nsAutoCString, which will
avoid heap allocation in the common case.

The patch also changes PREF_CopyCharPref() to PREF_GetCharPref(), because that
fits better now.

The |aPrefName| arguments in nsIPrefBranch.idl remain as |string| because they
almost always involve passing in C string literals, and passing "foo" is much
nicer than passing NS_LITERAL_CSTRING("foo").

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Update comm-central for nsIPrefBranch.idl changes

jorgk: as usual, I checked this compiled but nothing beyond that. It's all
pretty straightforward.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Change some |string| occurrences in nsIPrefBranch.idl to |AUTF8String|

Small update.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Change some |string| occurrences in nsIPrefBranch.idl to |AUTF8String|

I missed setCharPref().

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Update comm-central for nsIPrefBranch.idl changes

Updated version that handles the setCharPref() changes.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8921281 [details] [diff] [review]
Change some |string| occurrences in nsIPrefBranch.idl to |AUTF8String|

These changes look ok to me, but I don't know enough about the string API to grant a confident review. My only two questions are:

- Does changing the method definitions in the .idl have any potential side effects to the users of the API?

- Will this change the encoding of the resulting prefs file as it is written or read from the disk?

Comment 7

[Nicholas Nethercote [inactive]]

> - Does changing the method definitions in the .idl have any potential side
> effects to the users of the API?

It affects C++ users, as the patch indicates. It doesn't affect JS users; all the different string types are auto-converted to and from normal JS strings by XPConnect.
 
> - Will this change the encoding of the resulting prefs file as it is written
> or read from the disk?

No.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8921281 [details] [diff] [review]
Change some |string| occurrences in nsIPrefBranch.idl to |AUTF8String|

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

erahm: can you review? Thanks.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8921282 [details] [diff] [review]
Update comm-central for nsIPrefBranch.idl changes

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

I've looked at the entire patch and it all looked good to me, and like quite a bit of work. So THANK YOU a lot as always, that would have been nasty to fix as bustage at 1 AM at night, which is after the midnight merge. I did it last night in bug 1411069 :-(

::: mailnews/compose/src/nsSmtpServer.cpp
@@ -194,5 @@
>      if (NS_FAILED(rv))
>      {
>          rv = mDefPrefBranch->GetCharPref("hello_argument", aHelloArgument);
>          if (NS_FAILED(rv))
> -            *aHelloArgument = nullptr;

I wonder whether anyone checked for null here. Found it: nsSmtpProtocol.cpp: m_helloArgument.IsEmpty()) was already there.

::: mailnews/mime/src/mimetpla.cpp
@@ +70,5 @@
>      style.AppendLiteral("font-size: small; ");
>      break;
>    }
>  
> +  if (!citationColor.IsEmpty())

Converted null check.

@@ +221,5 @@
>    if (obj->closed_p) return 0;
>  
>    nsCString citationColor;
>    MimeInlineTextPlain *text = (MimeInlineTextPlain *) obj;
> +  if (text && !text->mCitationColor.IsEmpty())

Another converted null check here.

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8921281 [details] [diff] [review]
Change some |string| occurrences in nsIPrefBranch.idl to |AUTF8String|

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

Nice cleanup! There's one minor point, but otherwise r=me.

::: modules/libpref/Preferences.cpp
@@ +493,5 @@
>      return NS_ERROR_ILLEGAL_VALUE;
>    }
>  
> +  // It's ok to stash a pointer to the temporary PromiseFlatCString's chars in
> +  // pref because pref_HashPref() duplicates those chars.

Thanks for the comment.

@@ +498,3 @@
>    PrefValue pref;
> +  const nsCString& flat = PromiseFlatCString(aValue);
> +  pref.mStringVal = const_cast<char*>(flat.get());

Ugh, it would be nice to follow up some point and leave this const.

@@ -717,5 @@
>    PrefHashEntry* pref = pref_HashTableLookup(aPrefName);
>    return pref && pref->mPrefFlags.HasUserValue();
>  }
>  
> -// This function allocates memory and the caller is responsible for freeing it.

\o/

@@ +2483,5 @@
>                              const nsACString& aDefaultValue,
>                              uint8_t aArgc,
>                              nsACString& aRetVal)
>  {
> +  nsAutoCString utf8String;

Not sure if auto string is the right choice here, we're going to have to copy/alloc on assignment to |aRetVal|.

Comment 11

[Nicholas Nethercote [inactive]]

> @@ +498,3 @@
> >    PrefValue pref;
> > +  const nsCString& flat = PromiseFlatCString(aValue);
> > +  pref.mStringVal = const_cast<char*>(flat.get());
> 
> Ugh, it would be nice to follow up some point and leave this const.

Yeah, I have a follow-up for this.


> > +  nsAutoCString utf8String;
> 
> Not sure if auto string is the right choice here, we're going to have to
> copy/alloc on assignment to |aRetVal|.

Ok, I'll change to nsCString.

Comment 12

[Nicholas Nethercote [inactive]]

Also, I'm going to change all the AUTF8String types in the XPIDL to ACString. Turns out that libpref stores strings internally as Latin1, and if you try to pass them to JS as UTF-8 then XPConnect (correctly) barfs. ACString can handle Latin1.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0093f961eec929caa370444cd0f7fcbfbc82bc
Bug 1410794 - Change some |string| occurrences in nsIPrefBranch.idl to |ACString|. r=erahm.

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/23f3b04d3e13 - Mac plugin crashes, https://treeherder.mozilla.org/logviewer.html#?job_id=139393218&repo=mozilla-inbound, and Android exposed-interface complaints, https://treeherder.mozilla.org/logviewer.html#?job_id=139393835&repo=mozilla-inbound

Comment 15

[Florian Quèze [:florian]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> Also, I'm going to change all the AUTF8String types in the XPIDL to
> ACString. Turns out that libpref stores strings internally as Latin1, and if
> you try to pass them to JS as UTF-8 then XPConnect (correctly) barfs.
> ACString can handle Latin1.

When relanding, could you please fix the comment at https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0093f961eec929caa370444cd0f7fcbfbc82bc#l9.65 ? The getStringPref return type is actually AUTF8String.

Comment 16

[Nicholas Nethercote [inactive]]

> When relanding, could you please fix the comment at
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 3d0093f961eec929caa370444cd0f7fcbfbc82bc#l9.65 ? The getStringPref return
> type is actually AUTF8String.

I've already fixed it locally :)  Thank you for noticing!

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to Phil Ringnalda (:philor) from comment #14)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/23f3b04d3e13 - Mac
> plugin crashes,
> https://treeherder.mozilla.org/logviewer.html#?job_id=139393218&repo=mozilla-
> inbound,

These are ones that look like this:

> 20:53:45 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_pluginAudioNotification.html | Test timed out.
> 19940 20:53:46 INFO - TEST-UNEXPECTED-ERROR | dom/base/test/test_pluginAudioNotification.html | unexpected-crash-dump-found - This test left crash dumps behind, but we weren't expecting it to!
> 19943 20:53:46 INFO - TEST-UNEXPECTED-CRASH | dom/base/test/test_pluginAudioNotification.html | Finished in 305191ms
> 19969 20:53:49 INFO - TEST-UNEXPECTED-ERROR | dom/base/test/test_plugin_freezing.html | unexpected-crash-dump-found - This test left crash dumps behind, but we weren't expecting it to! 

I don't know what the problem is here. I don't see how prefs are related to these tests.


> and Android exposed-interface complaints,
> https://treeherder.mozilla.org/logviewer.html#?job_id=139393835&repo=mozilla-inbound

which look like this:

> 04:02:36 INFO - 883 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/test_worker_interfaces.html | false: If this is failing: DANGER, are you sure you want to expose the new interface NetworkInformation to all webpages as a property on the worker? Do not make a change to this file without a review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) 

I don't know what the problem is here, either. NetworkInformation's visibility is controlled by a pref, but it's a bool pref, and this patch only changed string prefs. Also, NetworkInformation is supposed to visible on Android. So again, I don't know what the problem is.

Comment 18

[Nicholas Nethercote [inactive]]

I worked out the problem; it involves Void strings.

With the old code, when GetCharPref() failed it wouldn't touch its `char** aRetVal` outparam. This outparam was usually handled through getter_Copies(), which would end up calling SetIsVoid(true) on the produced nsCString. The new code for GetCharPref() also does nothing with its `nsCString& aRetVal` outparam on failure, but the lack of getter_Copies() meant this nsCString isn't getting marked with SetIsVoid(true).

Furthermore, a few callers to GetCharPref() were then calling IsVoid() on the result without looking at the return value. Which is a bit nasty; assuming anything about the return value on failure is dangerous.

Anyway, the fix is for GetCharPref() to call SetIsVoid(true) in the failing case. This fixes the test failures.

Comment 19

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c906f68ee713d8dcf039b5617901035dfd69471
Bug 1410794 (attempt 2) - Change some |string| occurrences in nsIPrefBranch.idl to |ACString|. r=erahm.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c906f68ee71
(attempt 2) - Change some |string| occurrences in nsIPrefBranch.idl to |ACString|. r=erahm.

Comment 21

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5c906f68ee71

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5f5fbb45e801
Update comm-central for nsIPrefBranch.idl changes. r=jorgk

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1410794-follow-up.patch - C-C follow up

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fba3fa2958e7
Follow-up. rs=bustage-fix