826742 - nsExternalHelperAppService.cpp : Assertion: Empty aExtension parameter! (from DEBUG BUILD of Thunderbird)

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: nsExternalHelperAppService.cpp

While testing thunderbird (debug build of comm-central source) by
running "make mozmill" locally, I noticed many ASSERTION messages.

Among them is an assertion message from
nsExternalHelperAppSErvice.cpp.

From the cursory reading, I think the message comes from a function to
to find a proper helper application based on file extension (a la
".ppt", ".ps", etc. [not sure if "." is in the extension] )

The code is printing ASSERTION when such extension passed from the
caller is empty (because, I assume, such extension is non-existent for
a particular file name: i.e., "abc" vs "abc.ppt")

Such file names do exist and often are handed to us by some web
services, for example. (Usually the additional proper mime type,
etc. during downloading helps us in finding the appropriate helper
application.

So, although not getting the extension such as "ppt", or "ps" is
unlucky, but it is not fatal at all.  Just as the code does today,
simply returning without finding a matching application silently is
enough.

So I suggest that we remove NS_ASSERTION() statement.

I am proposing this change to remove noise from the session log of
"make mozmill" test run of DEBUG BUILD of TB.

Comment 1

[ISHIKAWA, Chiaki]

Comment on attachment 697947 [details] [diff] [review]
nsExternalHelperAppService.cpp

Not sure about whom to ask for review.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 697947 [details] [diff] [review]
nsExternalHelperAppService.cpp

No, this should be handled by the caller... do you have a stack trace?

Comment 3

[ISHIKAWA, Chiaki]

d(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #2)
> Comment on attachment 697947 [details] [diff] [review]
> nsExternalHelperAppService.cpp
> 
> No, this should be handled by the caller... do you have a stack trace?

I have a stack trace that shows the numerical address.
Somehow, the stack trace produced in stock "make mozmill" run
didn't show the symbols. (I have no idea why. Maybe it is in a JIT compiled code???).
Produced on Dec 28 (before the proposed patch was applied).
There were 13 ASSERTIONS, and all have the same
numerical address leading up to the assertion.

[Can someone tell me how to make this "stack trace" feature of mozmill test harness or the built-in function for debug trace to work using symbolic names if possible?]

I attach one full trace and a partial second one in the attachment.
The stack trace is like this.

###!!! ASSERTION: Empty aExtension parameter!: '!aExtension.IsEmpty()', file /TB-NEW/TB-3HG/new-src/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp, line 2765
UNKNOWN [/TB-NEW/TB-3HG/objdir-tb3/mozilla/dist/bin/libxul.so +0x01729AF8]
UNKNOWN [/TB-NEW/TB-3HG/objdir-tb3/mozilla/dist/bin/libxul.so +0x0172D2E4]
UNKNOWN [/TB-NEW/TB-3HG/objdir-tb3/mozilla/dist/bin/libxul.so +0x01B34248]
 ...

Comment 4

[ISHIKAWA, Chiaki]

Attachment: One full stack trace (and another to show the similarity)

The first one includes all the messages from the start of a test till the end of the "successful" completion (TEST-PASS) of the test.

The second one is partial and is only meant to show that the stack trace (albeit numerical) is the same with other case(s).


Re stack trace,  actually, as C and C++ source files are concerned,
the function is referenced only in the same file (!).
I checked this locally, and also 
the cross reference facility at mozilla says so, too.


https://mxr.mozilla.org/comm-central/ident?i=GetTypeFromExtras&filter=

 ...
Defined as a function prototype in:

    mozilla/uriloader/exthandler/nsExternalHelperAppService.h (View Hg log or Hg annotations)
        line 143 -- NS_HIDDEN_(bool) GetTypeFromExtras(const nsACString& aExtension, 

Referenced in:

    mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp (View Hg log or Hg annotations)
        line 2534 -- found = GetTypeFromExtras(aFileExt, aContentType);
        line 2700 -- bool found = GetTypeFromExtras(aExtension, type);
        line 2706 -- bool nsExternalHelperAppService::GetTypeFromExtras(const nsACString& aExtension, nsACString& aMIMEType) 
 
So the caller must be through these functions (or some kind of
XPCOM RPC magic.)

TIA

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Symbol stack trace (fixed by comm-central/mozilla/tools/rb/fix-linux-stack.pl

Sorry for taking such a long time.
I learned how to show the stack dump using symbols,
e.g.,
#
MOZ_OBJDIR=/TB-NEW/TB-3HG/objdir-tb3
# comm-central
MOZ_SRCDIR=/TB-NEW/TB-3HG/new-src

cd ${MOZ_OBJDIR}/mozilla/dist/bin
${MOZ_SRCDIR}/mozilla/tools/rb/fix-linux-stack.pl sessionlog  

I had to revert my change and compile, and get the log dump with the new binary,
and here it is. (I am still running the TB's make mozmill test, the binary running under valgrind(memcheck).
The first few test targets that show the ASSERTION are picked up and are included.

They all point to the same location.
My local copy may be a few lines off from the pristine comm-central source.
 
 2527	  // Ask OS.
 2528	  bool found = false;
 2529	  nsCOMPtr<nsIMIMEInfo> mi = GetMIMEInfoFromOS(EmptyCString(), aFileExt, &found);
 2530	  if (mi && found)
 2531	    return mi->GetMIMEType(aContentType);
 2532	
 2533	  // Check extras array.
*2534	  found = GetTypeFromExtras(aFileExt, aContentType);   <=== here.
 2535	  if (found)
 2536	    return NS_OK;
 2537	

The patch seems to be quite easy.

Is it OK to not to call GetTypeFromExtras when aFileExt is empty,
and continue to the following steps? I can create a patch easily.

TIA

Comment 6

[ISHIKAWA, Chiaki]

Attachment: proposed patch

Proposed patch.

This solved the problem of seeing the assertion error.

*BUT*, since I am not familiar with the code and so can't be sure,
I have a nagging feeling that we can simply return (or should simply return) 
failure, if aExtension is an empty string at the beginning of
GetTypeFromExtension().

TIA

Comment 7

[ISHIKAWA, Chiaki]

Attachment: Improved and tested patch take 2

>*BUT*, since I am not familiar with the code and so can't be sure,
>I have a nagging feeling that we can simply return (or should simply return) 
>failure, if aExtension is an empty string at the beginning of
>GetTypeFromExtension().

I did just that in this new patch, and I am leaving the old patch for comparison purposes. 

The local "make mozmill" test passed with flying colors.
I am checking it using tryserver now.

TIA

Comment 8

[ISHIKAWA, Chiaki]

> 
> The local "make mozmill" test passed with flying colors.
> I am checking it using tryserver now.
> 

It took me a while to sort out some compilation issues due to the different version of the compiler (GCC 4.7 on my local PC and 4.5 on TB tryserver).

The test passed and only experienced known random errors and so
I think the patch works and does not introduce any known problems.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9416d9b39821

TIA

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Basically the same patch landed in bug 874080. Though I'm wondering about the lack of nulling out aContentType. Paolo, is that something that should be done?

Comment 10

[:Paolo Amadini]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> Basically the same patch landed in bug 874080. Though I'm wondering about
> the lack of nulling out aContentType. Paolo, is that something that should
> be done?

I'm not particularly concerned here, though if we do this for the early failure
case, we should probably be consistent and null out the parameter at the end of
the function also. The current behavior seems undefined (the result can be null
or an empty string, or theoretically even some content type being evaluated,
I've not looked inside all the other functions we call there).

Comment 11

[ISHIKAWA, Chiaki]

(In reply to Paolo Amadini [:paolo] from comment #10)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> > Basically the same patch landed in bug 874080. Though I'm wondering about
> > the lack of nulling out aContentType. Paolo, is that something that should
> > be done?
> 
> I'm not particularly concerned here, though if we do this for the early
> failure
> case, we should probably be consistent and null out the parameter at the end
> of
> the function also. The current behavior seems undefined (the result can be
> null
> or an empty string, or theoretically even some content type being evaluated,
> I've not looked inside all the other functions we call there).

I would like to see a repetitive buggy behavior instead of
random undefined behavior if I have a choice although the patch in 874080 may fix the current issue after all.
So maybe I should create a patch that sets aContentType to null at the end of the function as well as setting aContentType in the early return case just in case
this undefined behavior bites us in the distant future.
What do you think?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I think that Paolo's a peer in this code and I'm not, so it's his call :P

Comment 13

[ISHIKAWA, Chiaki]

Oops, sorry. Requesting comment from Paulo.

Comment 14

[:Paolo Amadini]

(In reply to ISHIKAWA, Chiaki from comment #11)
> So maybe I should create a patch that sets aContentType to null at the end
> of the function as well as setting aContentType in the early return case
> just in case
> this undefined behavior bites us in the distant future.
> What do you think?

That's fine! If you can provide this patch and the link to a tryserver run
including only that patch, it will be a quick review.

Let's just make sure to mention the change in the developer documentation, just
in case external code relied on the buggy behavior, or empty string versus null.

Comment 15

[ISHIKAWA, Chiaki]

(In reply to Paolo Amadini [:paolo] from comment #14)
> (In reply to ISHIKAWA, Chiaki from comment #11)
> > So maybe I should create a patch that sets aContentType to null at the end
> > of the function as well as setting aContentType in the early return case
> > just in case
> > this undefined behavior bites us in the distant future.
> > What do you think?
> 
> That's fine! If you can provide this patch and the link to a tryserver run
> including only that patch, it will be a quick review.
> 
> Let's just make sure to mention the change in the developer documentation,
> just
> in case external code relied on the buggy behavior, or empty string versus
> null.

I created the patch and have tried to run it through thunderbird
TryServer with the patch alone.

However, there are tryserver issues right now.

xpcshell tests can not run due to some server configuration issue (known issue)
mozmill runs successfully except for a few known failures.
Win build fails due to another known issue.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=da43e4e05a08

Once these server issues are sorted out, I can re-run the test for review.

Comment 16

[ISHIKAWA, Chiaki]

Hi,

I could finally run thunderbird build run satisfactory after several days.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=f7bba1427ec4

Although, it has a few patches of my own, and a few more (from others) which I think fixed the TB tryserver bustage,
I can say that the  patch above

https://bugzilla.mozilla.org/attachment.cgi?id=731659


does not break anything [It did not introduce any more
bugs than known "permanent orange" errors.]

BTW, the patch for "ISHIKAWA, Chiaki – Originally noticed in bug 851917"
has an incorrect number. It should refer to 859197.
M-C patch 893362-fix is for aligning the module names during
build.

Unfortunately, linux (32 bits and 64 bits) have some configuration issue
on the tryserver and xpcshell test can't run under linux on TryServer.
XP and Win7 version showed that xpcshell ran successfully.
(Also, on my local PC, xpcshell test produced less errors (19) with the patches than untouched source files (28 error): there seem to be a few more permanent orange type errors in the linux version of xpcshell test suite output.)

I am afraid that xpcshell output under linux may need scrutiny once TB tryserver
can run it. I am afraid that there are permanent orage type errors, and probably
non-deterministic ones at that (sometime succeed, but fail other times). But this is outside the scope of this particular entry.

Comment 17

[:Paolo Amadini]

Let me know as soon as you can run a tryserver build that includes only this
patch, on top of a green mozilla-central or comm-central changeset.

Please run all tests on all platforms (except talos), as this change may impact
other code as well. Also, this generally makes it quite simple to identify
intermittent oranges.

Comment 18

[ISHIKAWA, Chiaki]

(In reply to Paolo Amadini [:paolo] from comment #17)
> Let me know as soon as you can run a tryserver build that includes only this
> patch, on top of a green mozilla-central or comm-central changeset.
> 
> Please run all tests on all platforms (except talos), as this change may
> impact
> other code as well. Also, this generally makes it quite simple to identify
> intermittent oranges.

Will do. That linux build can't run xpcshell-tests due to some tryserver configuration issues (it has been like this for some time) :-(
So maybe Mac OSX (derived from FreeBSD) test run would substitute for it. 

TIA

Comment 19

[Wayne Mery (:wsmwk)]

Chiaki, is this still an issue

Comment 20

[ISHIKAWA, Chiaki]

Please give me a couple of more days.
My PC has shown strange problems in the last four weeks. I get BSOD maybe a couple of times a day. :-(
I have replaced all my drives and network card. Still BSOD sometimes. (I run Debian GNU/Linux inside VirtualBox under Windows 10 Pro host).
So having a healthy windows 10 environment is essential.
I have run memory test. The result seems to be OK.
This only leaves maybe CPU, motherboard, or power supply as possible suspect. Ugh.

Comment 21

[ISHIKAWA, Chiaki]

I have found out that my proposed patch (in essentially the same form) has landed in the problematic code.
See comment 9, bug 874080.
So I think now this can be safely closed.