Bogus assertion: nsThread::kIThreadSelfIndex != 0

RESOLVED FIXED in mozilla1.8alpha1

Status

()

Core
XPCOM
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.8alpha1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

14 years ago
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;
  }
(Assignee)

Comment 1

14 years ago
Created attachment 148082 [details] [diff] [review]
v1 patch : remove bogus assertions
(Assignee)

Updated

14 years ago
Attachment #148082 - Flags: review?(timeless)

Comment 2

14 years ago
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

14 years ago
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.
Attachment #148082 - Flags: review?(timeless) → review+
(Assignee)

Comment 4

14 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8alpha

Comment 5

13 years ago
Regretfully, this bogus assertion is still in the firefox-1.0.6 -- the latest 
and greatest release of the browser... 
 
Too many source branches? 
(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

13 years ago
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... 
(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

13 years ago
> 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

13 years ago
(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

13 years ago
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

13 years ago
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... 
(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 drivers@mozilla.org, 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.

(and this patch, of course, wasn't an AMD64-specific patch)
You need to log in before you can comment on or make changes to this bug.