523672 - test_handlerService.js asserting on Linux debug unittests: ###!!! ASSERTION: index exceeds allowable range: 'i < mLength'

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[(not currently active) Ted Mielczarek]

Currently both test_handlerService.js and test_bookmarks.js are consistently crashing with what looks like the same stack on the Linux debug unittest runs. It's complicated by not having symbols for the stack (bug 519196).

Comment 1

[(not currently active) Ted Mielczarek]

Marco, Shawn, have either of you run this test on a debug Linux build?

Comment 2

[Marco Bonardo [:mak]]

hm, i did run all places toolkit tests on a linux debug build last month, but did not see anything strange.
Same stack for both? those tests are testing really different code paths...

Comment 3

[(not currently active) Ted Mielczarek]

Right, I assume the faulty code is lower in the platform somewhere, and both tests just happen to trigger it.

Comment 4

[(not currently active) Ted Mielczarek]

Ok, I reproduced these in a Linux debug build, they're separate issues. Not having symbols just makes it really tough to see! The test_bookmarks failure is bug 520445. The test_handlerService.js failure is different.

Comment 5

[(not currently active) Ted Mielczarek]

This is really lame. Inside the assertion here:
http://hg.mozilla.org/mozilla-central/annotate/7f1f309a34f8/uriloader/exthandler/unix/nsGNOMERegistry.cpp#l129
aFileExt is an empty string, so accessing aFileExt[0] hits the assertion here:
http://hg.mozilla.org/mozilla-central/annotate/7f1f309a34f8/xpcom/string/public/nsTSubstring.h#l235

CCing reed since he added that assertion.

Comment 6

[(not currently active) Ted Mielczarek]

Oh, I guess he just landed the patch in CVS. CCing Mike Hommey, who authored it.

Comment 7

[(not currently active) Ted Mielczarek]

Some more info, I think the bug is slightly up the stack. GetFromExtension is being called from here:
http://hg.mozilla.org/mozilla-central/annotate/7f1f309a34f8/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp#l83
but I think the GetPrimaryExtension() call right before that is failing, because mExtensions is an empty array:
http://hg.mozilla.org/mozilla-central/annotate/7f1f309a34f8/uriloader/exthandler/nsMIMEInfoImpl.cpp#l127

But GetHasDefaultHandler is ignoring the NS_ERROR_NOT_INITIALIZED return value and using the empty ext string.

Comment 8

[Mike Hommey [:glandium]]

(In reply to comment #6)
> Oh, I guess he just landed the patch in CVS. CCing Mike Hommey, who authored
> it.

IIRC, this has landed a while ago.

Comment 9

[Mike Hommey [:glandium]]

(In reply to comment #7)
> Some more info, I think the bug is slightly up the stack. GetFromExtension is
> being called from here:
> http://hg.mozilla.org/mozilla-central/annotate/7f1f309a34f8/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp#l83
> but I think the GetPrimaryExtension() call right before that is failing,
> because mExtensions is an empty array:
> http://hg.mozilla.org/mozilla-central/annotate/7f1f309a34f8/uriloader/exthandler/nsMIMEInfoImpl.cpp#l127
> 
> But GetHasDefaultHandler is ignoring the NS_ERROR_NOT_INITIALIZED return value
> and using the empty ext string.

Yes, it looks like this change is the culprit
http://hg.mozilla.org/mozilla-central/diff/99f24b52e463/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp

Comment 10

[Mike Hommey [:glandium]]

Ccing Wolfgang, who landed the patch.

Comment 11

[(not currently active) Ted Mielczarek]

Attachment: don't ignore failure

The "obvious" patch, just returning rv on failure, makes this test fail (but not assert), since it propagates up to the JS as an exception. Returning NS_OK here when we get NS_ERROR_NOT_INITIALIZED seems...weird, but it works and the test passes. I'm not sure if this is the right fix. Should we be propagating the NS_ERROR_NOT_INITIALIZED, and make the test expect it? What else would that break?

Comment 12

[(not currently active) Ted Mielczarek]

Mike, Wolfgang, does this patch look like the right solution?

Comment 13

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 407751 [details] [diff] [review]
don't ignore failure

That looks wrong.
We are only supposed to leave with NS_OK if we got an answer. If not we need to fall through to the next nsMIMEIfoImpl::GetHasDefaultHandler, don't we?

Comment 14

[Mike Hommey [:glandium]]

(In reply to comment #13)
> (From update of attachment 407751 [details] [diff] [review])
> That looks wrong.
> We are only supposed to leave with NS_OK if we got an answer. If not we need to
> fall through to the next nsMIMEIfoImpl::GetHasDefaultHandler, don't we?

I was about to tell you something like that: you should let the hildon and
mimeinfoimpl path go through.

I'd say setting mimeinfo to NULL when you get NS_ERROR_NOT_INITIALIZED would be
good.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yeah, the right fix here is to only call the GetFromExtension if the ext is nonempty.

Comment 16

[Mike Hommey [:glandium]]

(In reply to comment #14)
> I was about to tell you <snip>

I though that was Ted, sorry Wolfgang.

Comment 17

[(not currently active) Ted Mielczarek]

Someone else want to take this and do the right fix? I don't really know this code.

Comment 18

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

Attachment: only call GetFromExtension when we have an extension

Comment 19

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

http://hg.mozilla.org/mozilla-central/rev/5ec653ccfb52