243151 - Bogus assertion: nsThread::kIThreadSelfIndex != 0

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Darin Fisher]

The assertions here:

  http://lxr.mozilla.org/mozilla/source/xpcom/threads/nsThread.cpp#343

and here:

  http://lxr.mozilla.org/mozilla/source/xpcom/threads/nsThread.cpp#343

are completely bogus.  PR_NewThreadPrivateIndex may indeed return an index that
is 0.  in fact, the implementation of PR_NewThreadPrivateIndex confirms this:

  http://lxr.mozilla.org/mozilla/source/nsprpub/pr/src/threads/prtpd.c#85
  http://lxr.mozilla.org/mozilla/source/nsprpub/pr/src/threads/prtpd.c#85

the code in nsThread.cpp is already checking that PR_NewThreadPrivateIndex
returns PR_SUCCESS, so i think we should just remove the assertions.

if you want to see these assertions get hit, just run this program:

  int main()
  {
    NS_InitXPCOM2();
    NS_ShutdownXPCOM();
    return 0;
  }

Comment 1

[Darin Fisher]

Attachment: v1 patch : remove bogus assertions

Comment 2

[Wan-Teh Chang]

Comment on attachment 148082 [details] [diff] [review]
v1 patch : remove bogus assertions

I confirm that these two assertions are incorrect and
should be removed.

I checked the NSPR Reference and found that we do not
document the valid range of NSPR thread private data
indices, so one cannot assume that 0 is an invalid
NSPR thread private data index.

Comment 3

[timeless]

Comment on attachment 148082 [details] [diff] [review]
v1 patch : remove bogus assertions

ooh that sounds familiar, thanks.

one note:

http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsAutoLock.cpp#157

doesn't check. it makes an assertion in a comment, but it should at least
assert in code.

Comment 4

[Darin Fisher]

fixed-on-trunk

Comment 5

[Mikhail T.]

Regretfully, this bogus assertion is still in the firefox-1.0.6 -- the latest 
and greatest release of the browser... 
 
Too many source branches? 

Comment 6

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

(In reply to comment #5)
> Regretfully, this bogus assertion is still in the firefox-1.0.6 -- the latest 
> and greatest release of the browser... 
>  
> Too many source branches? 

No, as comment 4 says it is fixed on trunk. Therfore the fix will be in the next
major release i.e. Firefox 1.5 

Comment 7

[Mikhail T.]

I hate to sound negative, bitter, and abrasive, but comment 4 is well over a    
year old. There have been several releases of Firefox since then -- all of them    
containing this trivial to fix bug.    
    
The fact, that such patches take so long to travel from a branch to a branch,    
tells me, something is wrong. :-) 
    
On a more serious note, one is left to wonder, how many _other_ problems are    
already fixed on this never seen "trunk" over the past years, while the    
software that we, the users, deal with remains broken in some respect or the    
other. 
 
Perhaps, Mozilla would be wise to adopt FreeBSD's practice. There too, changes    
are first committed into the head branch (a.k.a. -current), but the CVS 
Template contains an "MFC After" field, which the committer is expected to fill 
out like: 
    
       MFC After: two weeks   
   
(MFC stands for "Merge From Current"). Scripts on the CVS server then see to   
it, that the committer (and whoever else) are reminded, when this selected  
"shake down" period elapses, so they can merge the change into the -stable 
branch(s) of the OS. 
 
Back to this particular problem, it may seem trivial (and ignorable) to you, 
but I'm trying to build a reliably working firefox on FreeBSD/amd64 and being 
distracted by bogus assertions and easily fixed compiler warnings is impeding 
my progress quite a bit -- _real_ bugs are much harder to spot... 

Comment 8

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

(In reply to comment #7)
> I hate to sound negative, bitter, and abrasive, but comment 4 is well over a    
> year old. There have been several releases of Firefox since then -- all of
them    
> containing this trivial to fix bug.    

All of these releases were made from a branch which only contains security updates.

> The fact, that such patches take so long to travel from a branch to a branch,    
> tells me, something is wrong. :-) 

Only security patches travel from trunk to branch.

> On a more serious note, one is left to wonder, how many _other_ problems are    
> already fixed on this never seen "trunk"

FWIW, the Deer Park releases are made from trunk.

Comment 9

[Mikhail T.]

> All of these releases were made from a branch which only 
> contains security updates. 
  
Seems like an overly conservative policy, which leads to bugs remaining unfixed  
and improvements remaining unseen and untested by the wider audience. 
  
I think, FreeBSD's approach provides for a better balance between stability and  
progress. Please, give it more thought. 
  
> FWIW, the Deer Park releases are made from trunk. 
 
But it is still considered "alpha", is not it? 

Comment 10

[James Ross]

(In reply to comment #7)
> I hate to sound negative, bitter, and abrasive, but comment 4 is well over a    
> year old. There have been several releases of Firefox since then -- all of
them    
> containing this trivial to fix bug.

So what if the comment is a year old? This is the fault of the stupid Firefox
developers who branched early [1] and then spent *months* working on branch-only
stuff for Firefox 0.9 and 1.0. All Firefox 1.0.x releases are from this branch,
and until the next full version finally comes out (as comment 6 says) there is
no official, final release of Firefox with it in.

Please make sure you know what is going on before making such comments in the
future.

[1] At 2004-05-15 according to the branch tag, though it seems that they used
source from a couple of weeks before that, as it did not have the 2004-05-15
version of nsThread.cpp in.

Comment 11

[Wan-Teh Chang]

Mikhail: you may want to raise this issue in a better forum
such as a mozilla.org newsgroup that its project management
team reads.  The mozilla.org project management team has
their reasons to be so conservative with maintenance branches.

This bug was most likely discovered by code inspection and
didn't affect anybody. The assertions are compiled away in
release builds.  I think this is why it's not necessary to
port the fix back to the Firefox 1.0 maintenance branch.

Comment 12

[Mikhail T.]

Alright. Is there an easy way to find amd64-specific commits, that went into 
the main branch, but not into firefox? 
 
I've fixed quite a few warnings here locally, but firefox still hangs and 
crashes on occasion. For example, trying to restart after installing the 
Forecastfox extension (5th most popular) hangs the process reliably... 

Comment 13

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

(In reply to comment #9)
> Seems like an overly conservative policy, which leads to bugs remaining unfixed  
> and improvements remaining unseen and untested by the wider audience. 

that that up with [e-mail], I guess.

> > FWIW, the Deer Park releases are made from trunk. 
>  
> But it is still considered "alpha", is not it? 

Yes, indeed.

(In reply to comment #10)
> [1] At 2004-05-15 according to the branch tag, though it seems that they used
> source from a couple of weeks before that, as it did not have the 2004-05-15
> version of nsThread.cpp in.

it branched off the 1.7 branch afaik

(In reply to comment #12)
> Alright. Is there an easy way to find amd64-specific commits, that went into 
> the main branch, but not into firefox? 

The patch did go into firefox, just not into the 1.0.x versions. I guess your
best bet is to query for resolved bugs containing amd64 in the summary or
something like that.

Comment 14

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

(and this patch, of course, wasn't an AMD64-specific patch)