789382 - nsIScriptError methods init() & initWithWindowID() should take nsAString, rather than wchar / PRUnichar*

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Daniel Holbert [:dholbert]]

As bz noted in bug 786108 comment 9, it's a little counterproductive that nsIScriptError's init() and initWithWindowID() methods take PRUnichar* -- they could just as easily take nsAString, which would be more abstract & could allow for string-sharing in some cases.

Filing this bug on making that change.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Patch 1: Change API & Impl

I'm splitting this into two patches, for (hopefully) clearer/easier reviewing:
  Patch 1) The changes to the IDL & the impl.
  Patch 2) The changes to all the client code.

I believe Patch 1 needs sr (as an API change), but patch 2 doesn't.

(I intend to fold the two patches together before I land, since Patch 1 by itself won't successfully build.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Patch 2: Changes to client code

This patch was pretty mechanical -- basically just:
  s/foo.get()/foo/
  s/rawString/nsDependentString(rawString)/
  s/nullptr/EmptyString()/

There are also a few static_casts sprinkled in, for strictness -- replacing reinterpret_casts and C-style casts, since we don't need the power of reinterpret_cast / c-style casts in those spots.

(I also simplified for consistency in a few places -- e.g. just calling NS_ConvertUTF8toUTF16() directly in mozJSComponentLoader.cpp's call to init(), since that's shorter & it's the pattern we follow almost everywhere.  Also one old nullcheck-after-new.)

CAVEAT: I haven't tested this yet; I've just confirmed that it successfully builds. :)

Comment 3

[Daniel Holbert [:dholbert]]

Note: The s/nullptr/EmptyString()/ changes are fine (won't change behavior) because nsScriptError::InitWithWindowID() just calls e.g. mSourceLine.Assign(sourceLine), and Assign() ends up truncating & returning when it's passed a null pointer:
https://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsTSubstring.cpp?mark=297-301#295

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Patch 1 v2: Change API & Impl

Forgot to rev the uuid -- fixed in this new version.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 659124 [details] [diff] [review]
Patch 1 v2: Change API & Impl

r=me

Comment 6

[Daniel Holbert [:dholbert]]

(I imagine this will break some comm-central code -- this will probably need a corresponding comm-central bug & patch.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 659123 [details] [diff] [review]
Patch 2: Changes to client code

Please test on try.  Going from const jschar* to const PRUnichar* used to require a reinterpret_cast on some platforms, iirc.

In the XBL_ProtoErrorReporter I'm a little worried about ucmessage or uclinebuf being null.  Can they be?  Some other places certainly null-check at least ucmessage.

Similar in the IndexedDatabaseManager.

And the component loader.

And XPCComponents.

And XPCConvert (just uclinebuf).

r=me with those fixed or the worry allayed.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 659123 [details] [diff] [review]
> Patch 2: Changes to client code
> 
> Please test on try.  Going from const jschar* to const PRUnichar* used to
> require a reinterpret_cast on some platforms, iirc.

I don't think that's an issue anymore -- in at least some of the patch's chunks, we were already using static_cast for that conversion, so that shouldn't cause problems. (that's what tipped me off that we could use reinterpret_cast, and I changed it for consistency)

(I'll look into the other null-checks & address them as-needed -- thanks for bringing that up! I'd assumed that nsDependentString(nsnull) would do the right thing & produce an empty string, but it looks like that'll assert, so we do need to be sure we're not passing in null there.)

FWIW: The (non-Android) Try builders didn't quite finish the full build process, due to issues in "make package" & friends, due to .jsm files needing updates to their nsIScriptError usage I believe.  For reference, the try push was:
 https://tbpl.mozilla.org/?tree=Try&rev=03bc6d4c751c
and from a quick grep, it looks like e.g.
 https://mxr.mozilla.org/mozilla-central/source/browser/devtools/tilt/TiltUtils.jsm#72
will need an update, and others as well.

So, I'll include a third patch for _JS_ client code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That's really odd.  JS should have been completely unaffected, since JS strings will Just Work and XPConnect will convert JS null to an empty AString....  I wonder whether the JS callers had "undefined" there or something?

Comment 10

[Daniel Holbert [:dholbert]]

I actually think the jsm files are fine after all -- it was likely that we were running a packaging tool *to package those files*, and that packaging tool involves one of the tweaked C++ classes, it crashed on a null pointer due to the null-checks mentioned in comment 7.

Comment 11

[Daniel Holbert [:dholbert]]

(or rather, due to the _lack_ of those null-checks)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2: Changes to client code [r=bz]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 659123 [details] [diff] [review]
> In the XBL_ProtoErrorReporter I'm a little worried about ucmessage or
> uclinebuf being null.  Can they be?

I think they can be. They're initialized to null (from the struct getting a memset(0)), in at least some cases, and we do have null-checks elsewhere, so might as well null-check them here.

So: added null-checks there.

> Similar in the IndexedDatabaseManager.

We're OK there, because both of the strings come from an nsString.  I've now just replaced the nsDependentString(buf) invocation with the actual nsString ("errorName") there, and I added an assertion that the other one is non-null (which we're guaranteed from how it's set, in FillScriptErrorEvent, here:
>  aEvent->fileName = mFilename.get();
https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBRequest.cpp?mark=227-227#223

And mFilename is a (concrete) nsString, declared here:
https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBRequest.h#103

so its .get() method should return non-null.

> And the component loader.

Added null-checks.

> And XPCComponents.

Added null-checks.

> And XPCConvert (just uclinebuf).

Added null-check for uclinebuf.

> r=me with those fixed or the worry allayed.

Thanks! Carrying forward r+, -- all of the above is fixed except for the IndexedDatabaseManager chunk, and hopefully I allayed your worries there?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Sounds good!

Comment 14

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70a0eda09c3c

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/70a0eda09c3c

Comment 16

[Philip Chee]

>(I imagine this will break some comm-central code -- this will probably need a
> corresponding comm-central bug & patch.)
Oh it did, and it does. Please file necessary comm-central bugs instead of waiting for us to discover stuff like this by having our trees catch fire.

Comment 17

[Daniel Holbert [:dholbert]]

Oops! Very sorry about that -- I forgot about that impact of this bug at some point after posting that comment.

I've filed bug 790157 to fix comm-central, and I can look into fixing it tomorrow morning.  Should I back out in the meantime?  (if so, I can do that in the morning)

(I don't see burning over on https://tbpl.mozilla.org/?tree=Thunderbird-Trunk , but maybe that's because this hasn't been merged there yet?  I don't really know how comm-central TBPL / build mechanics work.)

Comment 18

[Philip Chee]

> (I don't see burning over on https://tbpl.mozilla.org/?tree=Thunderbird-Trunk)
It appear to affect only Lightning. However Lightning is pretty integral to Thunderbird. See Bug 790015.