There is still the question of if I can upstream this to ICU, which I'm checking on.
In the meantime, Ryosuke Niwa has a good argument for option 3) above. I have uploaded a new patch to https://bugs.webkit.org/show_bug.cgi?id=88936that moves the NULL pointer check to inside the ICU-specific collator class. On Wed, Jun 13, 2012 at 1:14 PM, Ryosuke Niwa <rn...@webkit.org> wrote: > On Wed, Jun 13, 2012 at 12:48 PM, Myles C. Maxfield < > myles.maxfi...@gmail.com> wrote: > >> I think the fact that ICU thinks that a null pointer is an invalid string >> isn't necessarily an incorrect one. It's just a quirk of an interface to a >> library. I don't think that a good solution is to change the interface of >> ICU and try to upstream a patch to ICU >> > > Certainly not since not all browsers ship with their own ICU embedded > inside. > > I see two different solutions: >> 1) Document the fact that, in order to use webkit, you need an >> implementation of malloc(0) that doesn't return null pointers. This way, >> the burden of solving this problem is pushed downstream. >> > > I don't think we should impose arbitrary requirements like this. > > >> 2) Find some place along the every pipeline from malloc(0) to ICU, and >> arbitrarily modify the pointer to a non-zero value. This is probably the >> best solution, but a real fix of this nature requires finding all the >> places where pointers can be passed into ICU. >> > > We already have an abstraction layer for all ICU functions in WTF and > WebCore/platform because not all ports use ICU (e.g. Qt port). It should be > fairly easy to add an extra code there to check whether a null pointer is > passed in or not and bail out as appropriate. > > If I solve (via option 2) just my one particular occurrence of this >> problem, I see three different places to arbitrarily modify the pointer >> given to ICU: >> 1) change the m_copyData16 pointer in StringImpl to an arbitrary value, >> and check for that arbitrary value on destruction >> 2) change the characters() accessor to check if m_copyData16 is null, and >> return an arbitrary value if it is. This works because callers of >> characters() shouldn't ever delete the pointer nor dereference the pointer >> (since the string's length is 0) >> 3) check for null at the call site of the ICU function. >> >> I (perhaps prematurely) uploaded a CL >> implementing<https://bugs.webkit.org/show_bug.cgi?id=88936>option 2. What do >> you think? >> > > Adding any code to characters() will likely impact performance because it > tends to be used in hot code paths so I'd rather avoid that. > > - Ryosuke > >
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev