303801 - GetProperty in js/src/xpconnect/src/xpccomponents.cpp appears misused

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.7.10) Gecko/20050807 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.7.10) Gecko/20050807 Firefox/1.0.6

Normally you do not see this, but modified the file to avoid a compiler warning
(and to remove the needless variable). While there, I wondered, what is
GetProperty() to do, when given id does not match the two it is prepared to handle.

So I instrumented the function as per the attached patch. Apparently, the
function  is called once for every page (plus every time a mouse clicks inside a
browser's page) with a bogus ID:

###!!! ASSERTION: index out of range: 'index < IDX_TOTAL_COUNT', file
xpcprivate.h, line 574
Break: at file xpcprivate.h, line 574
id 6197108 ((null)) tells me (GetProperty) to do nothing

According to the debugger attached at run-time, this happens in the following stack:

(gdb) p id
$1 = 6197108
(gdb) n
1934
(gdb) where
#0  nsXPCComponents::GetProperty (this=0x52d000, wrapper=0x52d088, cx=0x5e8f74,
obj=0x568e80, id=6197108, 
    vp=0x7fffffffa148, _retval=0x7fffffff9f78) at xpccomponents.cpp:1934
#1  0x00000008039f9dc4 in XPC_WN_Helper_GetProperty (cx=0xa17c00, obj=0x7ad520,
idval=6197108, vp=0x7fffffffa148)
    at xpcprivate.h:1805
#2  0x000000080069cd19 in js_Interpret (cx=0xa17c00, result=0x7fffffffa258) at
jsinterp.c:2831
#3  0x000000080069f1a6 in js_Invoke (cx=0xa17c00, argc=1, flags=0) at jsinterp.c:972
#4  0x00000008006947e2 in js_Interpret (cx=0xa17c00, result=0x7fffffffa5f8) at
jsinterp.c:3000
#5  0x000000080069f1a6 in js_Invoke (cx=0xa17c00, argc=2, flags=1) at jsinterp.c:972
#6  0x0000000800691997 in js_Interpret (cx=0xa17c00, result=0x7fffffffa998) at
jsinterp.c:2606
#7  0x000000080069f1a6 in js_Invoke (cx=0xa17c00, argc=1, flags=2) at jsinterp.c:972
#8  0x000000080069f568 in js_InternalInvoke (cx=0xa17c00, obj=0xf54940,
fval=17158000, flags=2, argc=1, 
    argv=0x7fffffffad08, rval=0x39) at jsinterp.c:1049
#9  0x00000008006645c9 in JS_CallFunctionValue (cx=0xa17c00, obj=0x52d088,
fval=0, argc=5672576, 
    argv=0xffffffff805d98c0, rval=0x1) at jsapi.c:3698
#10 0x00000008066b9780 in nsJSContext::CallEventHandler (this=0xa11d00,
aTarget=0xf54940, aHandler=0x105cf70, 
    argc=1, argv=0x7fffffffad08, rval=0x7fffffffacd8) at nsJSEnvironment.cpp:1296

Seems like a waste of cycles at best, hmm?

Reproducible: Always

Comment 1

[Mikhail T.]

Attachment: fix compiler warning nicely and complain about bogus (?) id-values

Comment 2

[zug_treno]

Maybe this belongs to Product: Core - Component: XPConnect?

Comment 3

[Jaime Mitchell (use [e-mail] for email)]

Comment on attachment 191900 [details] [diff] [review]
fix compiler warning nicely and complain about bogus (?) id-values

You can not review your own patches, you must request review from the
apropirate peer.

Comment 4

[Mikhail T.]

(In reply to comment #2)  
> Maybe this belongs to Product: Core - Component: XPConnect?  
  
Quite possible. Please, feel free to change. I just want this fixed ASAP, so  
that the FreeBSD port does not need to be better than the rest of firefox  
distributions :-)  
  
(In reply to comment #3)  
> You can not review your own patches, you must request review 
> from the apropirate peer. 
 
I was recently informed (in a response to another bug report), that one must 
explicitly request reviews -- simply submitting patches is not enough in that 
person's opinion. So I expected marking "request review", but leaving the 
e-mail address empty to mean that I'd like the patch reviewd, but DO NOT KNOW 
BY WHOM. 
 
In other words, I expected the same mechanism that is used for automatically 
assigning bug reports to be used to automatically select a reviewer. 

Comment 5

[Jaime Mitchell (use [e-mail] for email)]

(In reply to comment #4)
> I was recently informed (in a response to another bug report), that one must 
> explicitly request reviews -- simply submitting patches is not enough in that 
> person's opinion. So I expected marking "request review", but leaving the 
> e-mail address empty to mean that I'd like the patch reviewd, but DO NOT KNOW 
> BY WHOM. 

You request review by setting to flag to ?, and someone else then grants it by
setting the flag to +. So you should set the request review to a ? rather than a
+. However it is best to try and find a specific person to request review from
as you are more likely to get the patch reviewed. The module peers list is a
good place to start or asking in #developers on moznet.

Comment 6

[Mikhail T.]

Just to be clear -- I'm seeing this in firefox-1.0.6, not in whatever is the 
latest and greatest HEAD version of the tree. 

Comment 7

[timeless]

Comment on attachment 191900 [details] [diff] [review]
fix compiler warning nicely and complain about bogus (?) id-values

i don't think NS_DEBUG is needed.

__func__ is almost certainly not portable enough for us, if necessary I can add
it to the list of things not to use in our portability list.

If you could use -p in your diff flags, that'd be nice. it'd be nicer still if
you could use cvs diff against cvs-mirror.mozilla.org. I know you're
religiously opposed, but if necessary, visit irc.mozilla.org, and ask someone
else to help you prepare the cvs diff.

Comment 8

[Brendan Eich [:brendan]]

Someone cc'ed me, so this must be worth looking at.  Wait, it's not!

- Yet another bogus gcc warning (where the compiler should know that res is used
only if doResult, and that doResult implies res was set).  Get a real compiler
(note MSVC does not have such a bug).  There was nothing incorrect with the code
as it stands without your patch, that your patch "fixes".
- The "waste of cycles" is not showing up on any profile, and while it could be
fixed for some unmeasurably small gain, there are much bigger fish to fry elsewhere.
- Learn the code better before patching it blindly.  There is no "misuse" here,
just a trivial inefficiency in using a generic-to-all-ids callback to implement
two ids.
- Don't file trivial bugs against branches that are maintained only for security
bugfixes.  Set the bug's Version field appropriately.

/be

Comment 9

[Mikhail T.]

(In reply to comment #8)       
> - Yet another bogus gcc warning (where the compiler should know that res       
> is used only if doResult, and that doResult implies res was set).       
       
Why, then, us doResult _at all_? For someone, who objected to initializing some       
variables as "inefficient" defending a use of a totally redundant one seems       
strange.       
       
This report, however, is not about fixing a warning (whatever that process'       
merits), but about a possible misuse of the function. It seems to be called too       
often with nothing to do.       
       
> Get a real compiler (note MSVC does not have such a bug).       
       
Once MSVC runs on a real Operating System, I'll consider it. Thank you.       
       
> There was nothing incorrect with the code       
> as it stands without your patch, that your patch "fixes".       
       
Yes, the compiler is wrong in this case, but I would not have bothered you with     
such a trivial matter. What made me file this report, again, is that the whole     
function appears misused. And not just for performance sake, but logically --     
it just does not seem right, that it is invoked with totally foreign ids and     
may point to a bug somewhere else.      
     
If -- in the educated opinion of the code's maintainers -- it is not, then, by       
all means, close the bug and leave the warning if you must spite gcc (and       
myself) with something.       
       
> - Learn the code better before patching it blindly.   
> There is no "misuse" here, just a trivial inefficiency in using a   
> generic-to-all-ids callback to implement two ids.    
    
What is the number 6197108? I have no idea -- a widget id? And are you sure, it   
will never happen to accidentally translate into one of the two numbers the   
function is prepared to recognize?  
  
> - Don't file trivial bugs against branches that are maintained only for   
> security bugfixes.  Set the bug's Version field appropriately.       
      
Must you really work yourself up into having to appologise every day?      
      
(In reply to comment #7)       
> __func__ is almost certainly not portable enough for us      
      
Yes, I understand. Perhaps an NS_ASSERT() or something should be used there      
(avoiding an explicit #ifdef DEBUG, et al.) On the second though, there is no 
need for GetStringName() either -- it will never succeed and passing it the raw 
id may be wrong anyway. 
 
But Brendan indicates (in no uncertain terms), there is not anything wrong with 
how this function is used. If you agree with him, please, just consider 
eliminating the redundant doResult -- if you do not fear Brendan's wrath, that 
is...     
 
> it'd be nicer still if you could use cvs diff against cvs-mirror.mozilla.org.     
     
Huh :-) Against which branch? Updating one file to its latest -rHEAD will often     
break things. Updating all may break some other parts of the tree. Or do you     
want diffs untested by an actual build? 
 
As for 'diff -p', I noticed, it does not always work right. But I'll try.   

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (In reply to comment #8)       
> > - Yet another bogus gcc warning (where the compiler should know that res       
> > is used only if doResult, and that doResult implies res was set).       
>        
> Why, then, us doResult _at all_? For someone, who objected to initializing
some       
> variables as "inefficient" defending a use of a totally redundant one seems       
> strange.       

I didn't "defend" it, I said there was nothing incorrect, as in broken, in the
code as it stood.  I also wrote that this is a gnat, not worth my time -- that
was directed at whoever cc'd me on this bug.

Furthermore, js_Interpret is the critical section of the JS engine.  This
callback is run infrequently and does not show up on profiles.  We should fix
bugs that actually have costs, in either correctness, performance, or some other
detectable or measurable quantity or quality, and in priority order related to
severity experienced by real users.

Sure, it's nice to clean up code.  I thought your patch, apart from the else
clause's use of __func__ and fprintf, was an improvement.  But it's not that big
a deal, and I didn't see the point in bothering about it.  If you keep up like
this, you *will* waste lots of others' time.  Please find higher priority stuff
(your 64-bit LiveConnect bug was good) to file bugs about.

> This report, however, is not about fixing a warning (whatever that process'       
> merits), but about a possible misuse of the function. It seems to be called too
> often with nothing to do.

Show me a profile where this matters.  Oh, you haven't profiled Firefox.

> > Get a real compiler (note MSVC does not have such a bug).       
>        
> Once MSVC runs on a real Operating System, I'll consider it. Thank you.

Hey, I liked BSD Unix best (I stopped hacking on Unix a decade ago), better than
Linux by comparison when I've read code -- but the nit-picking, low priority
misplaced focus is going to be the death of free Unix-like OSes, and Windows
will keep on being the OS you don't get fired for buying, if you're in a big
company.

In short, you might do better for BSD and Mozilla if you didn't bend either
around GCC's failures.
 
> What made me file this report, again, is that the whole     
> function appears misused. And not just for performance sake, but logically --     
> it just does not seem right, that it is invoked with totally foreign ids and     
> may point to a bug somewhere else.

What do you mean by "totally foreign"?  Do you know how JS's API works?  You
write "misused", but where is the "mis-" (as in "mistake")?  What is going
wrong, is id an invalid jsval?  Do you know how jsvals are used as ids?

> If -- in the educated opinion of the code's maintainers -- it is not, then, by
      
> all means, close the bug and leave the warning if you must spite gcc (and       
> myself) with something.       

I won't close the bug, because the code is as you say suboptimal.

However, this imperfection is of no significance to users, and I didn't write
this code, and I have no desire to spend time fixing it (although I could have
done that much more quickly than I am taking time fencing with you), because:
(a) right now is not the time for this sort of patch, so I'd have to keep it in
a tree till the Mozilla 1.9 milestone opens up on the trunk;
(b) I think it is necessary for me to argue with you on general principles,
here, to save time you might cause to be wasted in the future.

> > - Learn the code better before patching it blindly.   
> > There is no "misuse" here, just a trivial inefficiency in using a   
> > generic-to-all-ids callback to implement two ids.    
>     
> What is the number 6197108? I have no idea

Then read and learn, don't file and patch bugs.

Did you jump in a car and start turning switches and pressing pedals before
learning to drive?

> -- a widget id? And are you sure, it   
> will never happen to accidentally translate into one of the two numbers the   
> function is prepared to recognize?  

Yes, I am sure.

I'll ask this once: why don't you study the JS engine, particularly its tagged
pointer architecture, especially how ids are interned, before asking a rookie
question like this after positing that you've found a bug.  I'm not going to tie
up this bug with a Q&A session on the architecture of the engine.

> > - Don't file trivial bugs against branches that are maintained only for   
> > security bugfixes.  Set the bug's Version field appropriately.       
>       
> Must you really work yourself up into having to appologise every day?

No, and I didn't do anything to apologize for here.

Must you work yourself up over trivial code imperfections every day?

Where's your apology for your misreading of code and imputing bugs where there
are no bugs, wasting others' time?

Why are you trying to submit patches to old versions of code that got into a
distribution pipeline that will upgrade to the current trunk version, when you
could be testing and patching the current trunk version?

Are you really going to try replacing Firefox 1.0.x DSOs with patched builds of
your own?  That seems like anywhere from a silly to a very bad idea to me, and I
won't support you, even with comments like this one, if you keep it up.

/be

Comment 11

[timeless]

Comment on attachment 191900 [details] [diff] [review]
fix compiler warning nicely and complain about bogus (?) id-values

to be fair, the version field was my fault, it happened when i changed the
bug's product, i didn't notice that i was setting it to trunk. the cc was also
my fault. i had seen one of the other bugs (and by the time i'm commenting
again, i've seen more).

Comment 12

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.