616841 - String.localeCompare behaves incorrectly for international, uppercase and lowercase characters when called by an xpcom callback

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[Pavel Cvrcek [:JasnaPaka]]

1) Install Test Pilot (https://addons.mozilla.org/en-US/firefox/addon/13661/) and Czech Spell Dictionary (https://addons.mozilla.org/en-US/firefox/addon/3394/)
2) Open list of extensions in Add-ons Manager.
3) In list of installed extensions you will see order:

* Test Pilot
* Český slovník na kontrolu pravopisu

It's wrong order. Right variant is:

* Český slovník na kontrolu pravopisu
* Test Pilot

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hm, we already had this as bug 565757, which has been fixed a while back. Has this problem be regressed recently?

Comment 2

[Pavel Cvrcek [:JasnaPaka]]

I see this problem in latest FF 4.0 build. Maybe regression?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yeah, it's a regression since beta 7. There aren't that many fixes on trunk since beta 7 has been released:

https://bugzilla.mozilla.org/buglist.cgi?resolution=FIXED&query_format=advanced&bug_status=RESOLVED&component=Add-ons%20Manager&target_milestone=mozilla2.0b8

I could only think of bug 614416, or probably a side-effect of the new final theme landed with the patch on bug 601022. I will check.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Definitely a regression from bug 614416.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I wonder why the automated test on bug 565757 hasn't failed.

Comment 6

[Dave Townsend [:mossop]]

There is something odd going on. The test for bug 565757 is working, I can see when running it that the list is sorted correctly. Also strangely although I can reproduce this here, resorting the list by toggling the name sort fixes the ordering.

Comment 7

[Dave Townsend [:mossop]]

Attachment: broken code

This seems to be some kind of JS bug. I added this test code which is called when sorting the lists. When I first open the add-ons manager it shows the following output:

"Český slovníky pro kontrolu pravopisu".localeCompare("DOM Inspector") = 200
"DOM Inspector".localeCompare("Český slovníky pro kontrolu pravopisu") = -200

After that any attempt to resort the list displays:

"Český slovníky pro kontrolu pravopisu".localeCompare("DOM Inspector") = -2
"DOM Inspector".localeCompare("Český slovníky pro kontrolu pravopisu") = 2

This is pretty broken but I haven't yet managed to narrow down why it is changing behaviour.

Comment 8

[Dave Townsend [:mossop]]

Narrowed this down. String.localeCompare is broken when called by a callback from the add-ons manager but works when called by a DOM event listener. In xpcshell it is broken all the time. This seems like an xpconnect/JS issue so going to move over there.

Comment 9

[Dave Townsend [:mossop]]

Attachment: testcases

These are testcases that demonstrate the errors.

Comment 10

[Boris Zbarsky [:bzbarsky]]

If you're running on a Window, we set locale callbacks on the JSContext.  See the localeCallbacks struct in nsJSContext::nsJSContext.

I suppose we should do this for JS component contexts too in the component loader?  And perhaps for the safe js context?  I'm not sure which context we're actually entering script on in this case...

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Sounds like bug 441370.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Chris, this is the bug I mentioned.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sigh, fun bug.  AFAICT the web+extension-visible things affected are
 - JS components
 - xpconnect "safe context" (not sure what is, just listing per comment 10)
 - Web Workers
 - OOP jetpacks
 - out-of-process TabChild global, for message manager
 - in-process "TabChild global"
 - maybe ctypes, not sure
 - (pretty sure the localeCompare on |window| is broken in content processes)

and we would probably want to fix
 - xpcshell
 - IPC testshell

The locale service claims to be threadsafe (do we want to find out? ;) ), so we can theoretically fix all of the above except the out-of-process stuff; jetpack processes don't have XPCOM, and I don't believe content processes have access to localization files.

In light of this, is it true that only fixing
 - JS components and/or "safe context"
 - xpcshell

blocks betaN?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do these callbacks need access to localization files, or just access to intl stuff?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Quick skim suggests that they stay in intl's yard (not sure though).  With the workaround in bug 576507, this might bring localeCompare in content processes back into play.  OOP jetpack is still out of luck.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: WIP

First crack.  This patch has the included test case pass in xpcshell.  To fix the other 100 broken callsites, we would just need to add a similar test for each and sprinkle xpc_LocalizeContext() dust near the JS_NewContext().

I understand the lifetime of objects here and organization of this code not a bit, much appreciate suggestions there.  The biggest potential problems with this patch seem to me to be
 - having XPConnect stuff depend on XPCOM stuff.  Don't know the rules here.
 - the intl stuff is lazily init'd for each context.  Depending on how early localeCompare is needed, the initing could maybe possibly fail or lead to circular dependencies, not sure.
 - this patch assumes xpc_LocalizeContext needs to be thread safe, for simplicity uses PR_CallOnce to achieve it.  With this impl, init will only be attempted once.  That might not be a bad thing, but if thread safety isn't needed, or multiple init attempts need to be made, the guards should change.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> - having XPConnect stuff depend on XPCOM stuff.  Don't know the rules here.

XPConnect depends on XPCOM, period.  That's its job.  Depending on particular components is theoretically supposed to be conditional on !XPCONNECT_STANDALONE, but in practice that's dead.  So that part is fine.

> the intl stuff is lazily init'd for each context.

That's fine; that's what we had before, right?

> I understand the lifetime of objects here not a bit

The current setup before your patch is basically that while a JSContext that references these callbacks is live we don't release the decoder and collation.  That guarantees that things don't die while the callbacks can be invoked.

I think the right thing to do is probably to change callsites that xpc_LocalizeContext to notify xpconnect when they JS_DestroyContext that context.  That way xpconnect can keep track of the number of outstanding contexts referencing the callbacks, and do a similar game where if we're shutting down and the number is 0 or the number goes to 0 after we have shut down we do the release.

> - this patch assumes xpc_LocalizeContext needs to be thread safe

You mean that the locale callbacks need to be threadsafe, right?

In practice, since a single gDecoder is used for all callers, and since the decoding process uses members of the decoder and is totally not reentrant, the setup in the attached patch is limited to only being called on one thread, right?  I don't know offhand that gCollation has the same issue, but I wouldn't be all that surprised if it keeps some stuff in members too.

I'm not sure whether it's best to store a decoder and collation per thread using XPConnect's per-thread-data stuff (complicates lifetime management!) or whether we can get away with aborting if !NS_IsMainThread and these callbacks are called...  Given the general behavior of intl, I wouldn't be surprised if we just needto require the latter, though.  But that would mean that using locale stuff in a JS component running off the main thread would immediately abort.  Or I guess we could just throw a JS exception if these callbacks are called on a non-main thread.  Brendan, thoughts?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, if you meant having xpconnect depending on nsDOMClassInfo... yes, that's totally fine imo.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #18)
> > - having XPConnect stuff depend on XPCOM stuff.  Don't know the rules here.
> 
> XPConnect depends on XPCOM, period.  That's its job.  Depending on particular
> components is theoretically supposed to be conditional on
> !XPCONNECT_STANDALONE, but in practice that's dead.  So that part is fine.
> 

Yes, I meant a dependency on particular components.

> > the intl stuff is lazily init'd for each context.
> 
> That's fine; that's what we had before, right?
> 

For dom/base it's no different, yes.  But, in general, it's different.  I'm worried about a case where we might lazily "localize" a JSContext soon after InitXPCOM(), but then say soon thereafter we instantiate a JS component and during instantiation something uses localeCompare which creates some kind of instantiation cycle.  Or something.  Really, I just don't know the dangers here.  The WIP deals with failed intl setup, but never tries again if there's a failure.

> > - this patch assumes xpc_LocalizeContext needs to be thread safe
> 
> You mean that the locale callbacks need to be threadsafe, right?

Well, xpc_LocalizeContext itself and the lazy init code in the callbacks, which touches global state.  LocalizeContext itself is probably OK unless there's some JS request needed.  My patch is threadsafe wrt to lazy init.

> 
> In practice, since a single gDecoder is used for all callers, and since the
> decoding process uses members of the decoder and is totally not reentrant, the
> setup in the attached patch is limited to only being called on one thread,
> right?  I don't know offhand that gCollation has the same issue, but I wouldn't
> be all that surprised if it keeps some stuff in members too.
> 
> I'm not sure whether it's best to store a decoder and collation per thread
> using XPConnect's per-thread-data stuff (complicates lifetime management!) or
> whether we can get away with aborting if !NS_IsMainThread and these callbacks
> are called...  Given the general behavior of intl, I wouldn't be surprised if
> we just needto require the latter, though.  But that would mean that using
> locale stuff in a JS component running off the main thread would immediately
> abort.  Or I guess we could just throw a JS exception if these callbacks are
> called on a non-main thread.

I skimmed the necessary modules a couple weeks ago and recall them claiming to be threadsafe.  I don't know if that's true.  The locale callbacks do need to be threadsafe regardless of off-main-thread JS components because one class of broken JSContexts is those created for web workers.  But in general I guess we could leave JSContexts selectively broken.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I'm worried about a case where we might lazily "localize" a JSContext soon
> after InitXPCOM(), but then say soon thereafter we instantiate a JS component
> and during instantiation something uses localeCompare which creates some kind
> of instantiation cycle.

Right; so the worry is basically that some JS component will try to use the locale stuff before things are set up enough that intl components can be instantiated?

> I skimmed the necessary modules a couple weeks ago and recall them claiming to
> be threadsafe.

Really?  I don't see how any single unicode decoder can possibly be threadsafe... nsUTFToUnicode, for example, certainly isn't.

> The locale callbacks do need to be threadsafe

Then they probably need to either use a per-thread decoder or lock around the decoder use, at least....  I dunno about collation, like I said.

But yes, we could also just not worry about workers for now and leave them broken.  I'm not sure whether that's better than having the locale stuff throw in workers, though...

Comment 22

[Boris Zbarsky [:bzbarsky]]

> nsUTFToUnicode, for example

I meant nsUTF8ToUnicode

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #21)
> > I'm worried about a case where we might lazily "localize" a JSContext soon
> > after InitXPCOM(), but then say soon thereafter we instantiate a JS component
> > and during instantiation something uses localeCompare which creates some kind
> > of instantiation cycle.
> 
> Right; so the worry is basically that some JS component will try to use the
> locale stuff before things are set up enough that intl components can be
> instantiated?
> 

Yes.

> > I skimmed the necessary modules a couple weeks ago and recall them claiming to
> > be threadsafe.
> 
> Really?  I don't see how any single unicode decoder can possibly be
> threadsafe... nsUTFToUnicode, for example, certainly isn't.
> 

Oh geez, I've been on another planet ... I looked at the modules needed to get to the compare+convert objects, I didn't even think about those themselves being non-pure.  And of course they might be implemented by extensions &c I guess.  Sigh.

Isn't the JS interface pure though?  Are we missing Reset() calls in here?  Anyway, more of an aside.

> > The locale callbacks do need to be threadsafe
> 
> Then they probably need to either use a per-thread decoder or lock around the
> decoder use, at least....  I dunno about collation, like I said.

I guess we could try per-thread, but I don't know how well that would work in full XPCOM generality.  (Too late to back that out? ;) )  Locking would leave us to open to easy unintentional DoS.

> But yes, we could also just not worry about workers for now and leave them
> broken.  I'm not sure whether that's better than having the locale stuff throw
> in workers, though...

I'm OK with trying a per-thread impl and seeing what blows up.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Isn't the JS interface pure though? 

Yes.

> I'm OK with trying a per-thread impl and seeing what blows up.

OK, let's do that.  We just have to make sure that the init and destroy for the JSContext happen on the same thread, then, so we can refcount each thread's stuff individually or something....  The other option would be to try to hang a pointer to the right things to refcount off the jscontext itself, but that might take too much surgery.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Actually, I just realized that workers aren't tied to a particular thread.  So that could throw a wrench in the works....

If the collation stuff is really threadsafe, we could also fix localeCompare in general but leave toLocaleString broken on non-main threads.  And file bugs on intl to make it easy (as in fast) to stack-instantiate a Unicode decoder; at that point we could just do it on every call or something.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Er, unicode encoder.  But same deal!

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #24)
> The other option would be to try to hang a
> pointer to the right things to refcount off the jscontext itself, but that
> might take too much surgery.

What's your worry here?

My current (possibly somewhat evil) thought is to
 - subclass |struct JSLocaleCallbacks|, add decoder and collation members
 - heap-allocate this subclass and init members from xpc_LocalizeContext
 - |JS_SetLocaleCallbacks(subclassInstance)|
 - set up a context callback that listens for context destruction
 - delete the subclass instance on destruction

The nice thing about this is there's nothing to refcount (well, nontrivially anyway).  Bad things are
 - unsure about JSContext destruction semantics; callback might be invoked at awkward time
 - depends on context callback not being stomped (i.e. chained); dunno if current users maintain that discipline
 - decoder/collation can be called on arbitrary number of threads, though not concurrently or reentrantly AFAIK

Maybe there's a more "official" of setting per-context data?  I'll poke at this too.

> If the collation stuff is really threadsafe, we could also fix localeCompare in
> general but leave toLocaleString broken on non-main threads.  And file bugs on
> intl to make it easy (as in fast) to stack-instantiate a Unicode decoder; at
> that point we could just do it on every call or something.

Yes, fast stack allocation of these would solve all problems.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> What's your worry here?

There wasn't an obvious worry; just wasn't sure how easy it is to hang things off the jscontext.

Also, that setup involves allocating a new collation and decoder for each window... but I think that should be ok.

> callback might be invoked at awkward time

in what sense?

> depends on context callback not being stomped

Worth checking.

> decoder/collation can be called on arbitrary number of threads

I _think_ that this should be ok....  They have threadsafe refcounting, and as long as you don't reenter things are fine.

> Maybe there's a more "official" of setting per-context data? 

Contexts can have a private pointer, but that's already in use in various cases (e.g. for windows the private pointer points to the nsJSContext).

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #28)
> > What's your worry here?
>
> Also, that setup involves allocating a new collation and decoder for each
> window... but I think that should be ok.
>

And worker etc.  Yeah, they seem small-ish.
 
> > callback might be invoked at awkward time
> 
> in what sense?
> 

I don't know what the contract for JS_DestroyContext() and the context callback is.  If it and the callback can be legally called in the middle of operations, then deleting the decoder/collation might cause bad things to happen.

> > decoder/collation can be called on arbitrary number of threads
> 
> I _think_ that this should be ok....  They have threadsafe refcounting, and as
> long as you don't reenter things are fine.
> 

I see that nsCollationUnix has an nsCollation which has an nsIUnicodeEncoder member, and nsUnicodeToUTF8 at least isn't IMPL_THREADSAFE.  Can't really tell why though; doesn't look materially different than nsUTF8ToUnicode.  Maybe other encoders have issues?  Could feel this out on tryserver+nightlies maybe.

> > Maybe there's a more "official" of setting per-context data? 
> 
> Contexts can have a private pointer, but that's already in use in various cases
> (e.g. for windows the private pointer points to the nsJSContext).

OK.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bent tells me that JSContexts created for Workers are currently tied to a single thread, although soon the contexts might be switched to being destroyed only on the main thread.

If we can assume that contexts are usually parked on particular threads, we should be able to hang the collation+decoder off JSContexts with no issues.  bent also points out JS_SetContextThread(), but the only interesting m-c caller is ctypes and I'm not sure we care about locale callbacks there.

Maybe we can xpc_LocalizeContext() Worker contexts for now, with the above approach, and if their destruction model changes, resume this chat in that bug?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> I don't know what the contract for JS_DestroyContext()

Brendan?  You listening?

> and nsUnicodeToUTF8 at least isn't IMPL_THREADSAFE.

:(

> Maybe we can xpc_LocalizeContext() Worker contexts for now, with the above
> approach, and if their destruction model changes, resume this chat in that
> bug?

That seems like the right way forward, esp. if we add some asserts that would fail if the model changes.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #31)

> > Maybe we can xpc_LocalizeContext() Worker contexts for now, with the above
> > approach, and if their destruction model changes, resume this chat in that
> > bug?
> 
> That seems like the right way forward, esp. if we add some asserts that would
> fail if the model changes.

Better believe it! ;)  nsUnicodeToUTF8 would catch it too.

Comment 33

[Brendan Eich [:brendan]]

Re: comment 29:

> I don't know what the contract for JS_DestroyContext() and the context callback
> is.  If it and the callback can be legally called in the middle of operations,
> then deleting the decoder/collation might cause bad things to happen.

What do you mean by "in the middle of operations"? You cannot destroy a context you're running script on -- hope that's obvious from the mandatory ownership (New and Destroy API verbs) model.

/be

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Well, let me be more specific.  My patch does approximately

  ContextCallback(cx, op):
    if op == JSCONTEXT_DESTROY:
      JSLocaleCallbacks* lc = JS_GetLocaleCallbacks(cx);
      JS_SetLocaleCallbacks(cx, nsnull);
      delete static_cast<FooGummy*>(lc);

Is that OK?

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looks like workers are probably out for FF4, unless anyone has other ideas:

###!!! ASSERTION: nsPosixLocale not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /home/cjones/mozilla/mozilla-central/intl/locale/src/unix/nsPosixLocale.cpp, line 48
nsPosixLocale::Release() (/home/cjones/mozilla/mozilla-central/intl/locale/src/unix/nsPosixLocale.cpp:48)
~nsCOMPtr (/home/cjones/mozilla/ff-dbg/intl/locale/src/unix/../../../../dist/include/nsCOMPtr.h:534)
nsCollationUnix::Initialize(nsILocale*) (/home/cjones/mozilla/mozilla-central/intl/locale/src/unix/nsCollationUnix.cpp:144)
nsCollationFactory::CreateCollation(nsILocale*, nsICollation**) (/home/cjones/mozilla/mozilla-central/intl/locale/src/nsCollation.cpp:67)
XPCLocaleCallbacks::Compare(JSContext*, JSString*, JSString*, jsval_layout*) (/home/cjones/mozilla/mozilla-central/js/src/xpconnect/src/xpclocale.cpp:241)
XPCLocaleCallbacks::LocaleCompare(JSContext*, JSString*, JSString*, jsval_layout*) (/home/cjones/mozilla/mozilla-central/js/src/xpconnect/src/xpclocale.cpp:127)
str_localeCompare (/home/cjones/mozilla/mozilla-central/js/src/jsstr.cpp:1008)
js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, js::Value*) (/home/cjones/mozilla/mozilla-central/js/src/jscntxtinlines.h:685)
CallCompiler::generateNativeStub() (/home/cjones/mozilla/mozilla-central/js/src/methodjit/MonoIC.cpp:691)
js::mjit::ic::NativeCall(js::VMFrame&, js::mjit::ic::CallICInfo*) (/home/cjones/mozilla/mozilla-central/js/src/methodjit/MonoIC.cpp:898)

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Refactor use of JSLocaleCallbacks to make them easier to share among various JSContexts

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: "Localize" the JS component JSContext

Not sure if I need to get the "safe context" here too, but this fixes Mossop's tests.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: "Localize" the messageManager JSContexts

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: "Localize" the IPC testshell JSContext

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Dead end test + patch for Workers

Patch makes the test work, but also trips threadsafety assertions pasted above.  Maybe we should spin this off into another bug?  Not sure that'd be a 4.0 blocker.

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 501263 [details] [diff] [review]
part 4: "Localize" the IPC testshell JSContext

Looks good to me.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 501260 [details] [diff] [review]
part 1: Refactor use of JSLocaleCallbacks to make them easier to share among various JSContexts

In the documentation for This() when talking about binding to the calling thread, you need s/MaybeThis/This/.

>+  nsAutoPtr<XPCLocaleCallbacks> lc(new XPCLocaleCallbacks());
>+  JS_SetLocaleCallbacks(cx, lc.forget());

Why do you need the |lc| temporary?  Why not just do:

  JS_SetLocaleCallbacks(cx, new XPCLocaleCallbacks());

?

r=me with those nits.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 501261 [details] [diff] [review]
part 2: "Localize" the JS component JSContext

r=me

You probably don't need to mess with the safe jscontext.  Or so I hope....

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

> In the documentation for This() when talking about binding to the calling
> thread, you need s/MaybeThis/This/.

Done.

> >+  nsAutoPtr<XPCLocaleCallbacks> lc(new XPCLocaleCallbacks());
> >+  JS_SetLocaleCallbacks(cx, lc.forget());
> 
> Why do you need the |lc| temporary?  Why not just do:
> 
>   JS_SetLocaleCallbacks(cx, new XPCLocaleCallbacks());

Hm not sure what I had in mind there.  Fixed.

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #38)
> Created attachment 501262 [details] [diff] [review]
> part 3: "Localize" the messageManager JSContexts

Review ping.  (Two-liner! ;) )

Comment 47

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

I was not CC'ed so pinging didn't help.

Comment 48

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No worries.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/a8f84cf5447e
http://hg.mozilla.org/mozilla-central/rev/a2cc2667bd9e
http://hg.mozilla.org/mozilla-central/rev/7a00523aff7e
http://hg.mozilla.org/mozilla-central/rev/94fa9ccf5d5a

Comment 50

[Craig Topper]

I believe this broke disable-libxul builds

Undefined symbols:
  "xpc_LocalizeContext(JSContext*)", referenced from:
      _main in xpcshell.o
ld: symbol(s) not found

Comment 51

[(no longer active)]

This broke non-libxul builds, I filed bug 626138 for that.

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110116 Firefox/4.0b10pre. Tested the steps in comment 0 and non-capital first letters.

Given the automated test I don't think we need a manual test. Is that correct Chris?

Comment 53

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't know of any reason to add a manual test.