47846 - nsStdUrl->Equals() doesn't work with nsMsgMailNewsUrl

Status: RESOLVED DUPLICATE of bug 98626

(Core :: CSS Parsing and Computation, defect, P3)

Description

[selmer (gone)]

2000-08-04-04-M17

Talkback ID = TB15374322M

Probably not a front-end thing.  Read a couple mails, deleted a few mails,
clicked on a new message and crashed.

Comment 1

[scottputterman]

does this happen all of the time or just once?

Comment 2

[leger]

This is all I could find in Talkback:

ReleaseSheet [d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSLoader.cpp, 
line 483] 
_hashEnumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp, line 100] 
PL_HashTableEnumerateEntries [plhash.c, line 414] 
nsHashtable::Enumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp, 
line 238] 
0x08458b51 

Comment 3

[selmer (gone)]

I hit it several times in a row, but it may be the same message causing it.  I 
was able to get the "same" crash by dragging a link out of the message (using 
4.5) into a Seamonkey browser window.  
(URL=http://www.oreillynet.com/pub/a/network/2000/06/30/magazine/mozilla_game.ht
ml)

Other talkback IDs = TB15374433Z and TB15378464E

Comment 4

[selmer (gone)]

I should mention that this was on Win98.

Comment 5

[scottputterman]

reassigning to layout.

Comment 6

[scottputterman]

*** Bug 46293 has been marked as a duplicate of this bug. ***

Comment 7

[leger]

Moving from [nsbeta2+] to [nsbeta2-], putting on nsbeta3 nominee radar.  Missed 
beta2 train...need 100 reproducible test case...this is sooo random. ;-)

Comment 8

[Pierre Saslawsky]

Reassigned to selmer to provide more complete stack traces. Or were they in the 
Talkbacks reports? I don't know how to access that either...

Comment 9

[laurel]

My bug #46293 was marked a duplicate of this.  So I'm providing current
feedback... I'm still seeing crash when I select a message with a large web page
attachment then select another message.  Mac and NT talkback reports attached.

Comment 10

[laurel]

Attachment: Mac talkback 15906991

Comment 11

[laurel]

Sorry, NT talkback report no good.

Comment 12

[lchiang]

Can we reproduce from scratch by sending the msg with the large attachment and
then following Laurel's steps?  Can we attach the attachment to this bug report?

Comment 13

[laurel]

The attachment is http://www.cnn.com.  It caused the problem awhile back with
the then current content and I attached it again today with its current content.
I don't know that we need to attach cnn.com to this bug report.

Comment 14

[selmer (gone)]

I am not the correct owner for this bug.  There are stack traces attached to
this bug.

Comment 15

[laurel]

Hello? What's happening with this bug (mine, bug 46293 was marked duplicate)?
Beta3 decision?

Comment 16

[lchiang]

chrisd - can you offer more insight into this bug? If it's a layout bug, do you
know what element may be the cause of the crash when displaying the msg content?

Comment 17

[laurel]

Hey, folks?  Still no comments from development and PDT here... in the light of
getting some more attention, I have removed the "NEW-REPRODUCIBLE ?" from the
status whiteboard field.  I think we've supplied that info... 

Comment 18

[selmer (gone)]

Adding [e-mail] to CC.  Any current status on this bug?  Will it be
plus or minus?

Comment 19

[karnaze (gone)]

Adding nsbeta3+ to status whiteboard.

Comment 20

[Michael La Guardia]

PDT agrees P1

Comment 21

[selmer (gone)]

brackets on nsbeta3+ in case it matters to any queries.

Comment 22

[laurel]

I'm hitting this a lot lately with various attached pages.  Really confuses
other testing... could someone please make sure this is being addressed? I don't
see any developer comments.

Putting in relnote3 keyword, since it may be this will not be fixed in time for
beta? 

Comment 23

[selmer (gone)]

Pierre, any update on this bug?  Thx.

Comment 24

[laurel]

Attachment: talkback incident 17681015

Comment 25

[laurel]

Attached a stack trace from a crash on NT with sept19 commercial build. 
Stack traces in many of the talkbacks for this problem today are coming up only
with a single stack line, such as this: 
 Call Stack:    (Signature = 0x0f25049f 322329b7) 
       0x0f25049f

Comment 26

[Pierre Saslawsky]

selmer: I started to work on it but I did not focus as much as I should have 
because I have the bad habit to work on the bugs whose approvals are questionable 
before working on those, like this one, that I know I will be allowed to check 
into the branch up to 20 minutes before we ship. This is a very bad habit, I 
know, I really should stop doing that!

Comment 27

[lchiang]

I have an email which causes this crash.  If you want the email, let me know.  
1.  Select message with particular attachment
2 [details] [diff] [review].  Select another message.
3.  Crash occurs.

Comment 28

[laurel]

Nominating rtm:  This is a really easy crash to hit, looks like it will not be
fixed in time for beta3.  

Please no more comments about how to reproduce, we have tons of that info.

Comment 29

[karnaze (gone)]

Marking rtm+.

Comment 30

[Michael La Guardia]

per laurel's comments, marking nsbeta3-.

Comment 31

[selmer (gone)]

converting [rmt+] to [rtm+]

Comment 32

[Jay Patel [:jay]]

I found this entry in the talkback data today:

gkhtml.dll + 0x74173 (0x60234173) d8a839fc
         line 
        Build: 2000092609 CrashDate: 2000-09-26 UptimeMinutes: 3  Total: 85 
        OS: Windows NT  4.0 build 1381
        URL: 
        Comment: this is bug http://bugzilla.mozilla.org/show_bug.cgi?id=47846
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=18085948
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=18085948

Here is the top of the stack trace:

Incident ID 18085948 
gkhtml.dll + 0x74173 (0x60234173) 
xpcom.dll + 0xbfc2 (0x60cabfc2) 
plds4.dll + 0x193d (0x60c8193d) 
xpcom.dll + 0xbfa3 (0x60cabfa3) 
gkhtml.dll + 0x740ed (0x602340ed) 
gkhtml.dll + 0x7409d (0x6023409d) 
gkhtml.dll + 0x74204 (0x60234204) 
gkhtml.dll + 0x6c313 (0x6022c313) 
gkhtml.dll + 0x6c0c2 (0x6022c0c2) 
gkhtml.dll + 0x98e2 (0x601c98e2) 
gkhtml.dll + 0xc52b8 (0x602852b8) 
jsdom.dll + 0x10829 (0x60b20829) 

If this crash is really related to this bug, please confirm...this crash has 
shown up in the topcrash list.

Comment 33

[Pierre Saslawsky]

I have a temporary patch. The problem happens when displaying a mail message with 
an attachment which loads an external stylesheet. It happens for instance when 
displaying a mail with a CNN page as an attachment.

The sequence is the following:
- the mail is loaded and the external stylesheet is fetched
- while the stylesheet is loading, it is added to a hash table in the css loader
- when the stylesheet is loaded, the completion routine tries to retrieve the 
loading stylesheet from the hash table
==> Problem #1: To load the stylesheet, we use a standard URI but when the 
completion routine is called, we receive a nsMsgMailNewsUrl. 
==> Problem #2: To retrieve the stylesheet from the hash table, the css loader 
uses nsIURI->Equals(nsIURI *other, PRBool *_retval) which calls 
nsStdURL::Equals() which uses the second URI to make a QueryInterface() on the 
nsStdURL IID **ONLY**.
==> Problem #3: nsMsgMailNewsUrl doesn't implement the nsStdURL interface, just 
the nsIURL interface.
==> Result: the QueryInterface fails, the hash table entry for the stylesheet is 
not cleared, still the stylesheet is deleted. Next time a message is displayed, 
the css loader is deleted and it crashes because its hash table points to invalid 
elements.

To fix that, I changed the test used to compare the 2 URIs: instead of calling 
nsIURI->Equals(), I get the strings with GetSpec() and do a simple strcasecmp().

Of course, it would be better to fix either nsIURI->Equals() or the inheritance 
of nsMsgMailNewsUrl but it is a bit trickier than it looks, so I'm going to let 
the original authors (warren, gagan and mscott) take care of it.

The patch is below, if you want to review it.


Index: nsCSSLoader.cpp
===================================================================
RCS file: /m/pub/mozilla/layout/html/style/src/nsCSSLoader.cpp,v
retrieving revision 3.60
diff -r3.60 nsCSSLoader.cpp
114a115
> #if 0 // bug 47846
117a119,125
> #else
>     nsXPIDLCString str1;
>     nsXPIDLCString str2;
>     mURL->GetSpec(getter_Copies(str1));
>     key->mURL->GetSpec(getter_Copies(str2));
>     return ((nsCRT::strcasecmp(str1, str2) == 0) ? PR_TRUE : PR_FALSE);
> #endif

Comment 34

[Warren Harris]

I'd prefer to fix nsStdURL::Equals instead so that others don't trip on the 
same problem. Can you come up with a patch for it?

Comment 35

[Michael La Guardia]

Marking "needinfo". This seems like a bad thing, but we need a patch, review and
super review to clear for checkin.

Comment 36

[selmer (gone)]

mscott, can you make any recommendation here on the existing patch vs Warren's
suggestion?

Comment 37

[Pierre Saslawsky]

The real fix would involve changing the inheritance of nsMsgMailNewsUrl.
That's for mscott. In the meanwhile, if someone wants my patch, fine.

Comment 38

[Pierre Saslawsky]

Clearing "need info". The patch from [2000-09-28 07:24] was reviewed by attinasi 
last week and brendan today.

Comment 39

[Phil Peterson]

PDT marking [rtm++]

Comment 40

[Pierre Saslawsky]

Fix checked in nsCSSLoader.cpp (trunk + Netscape_20000922_BRANCH).

Cleared the Keywords and the Status Whiteboard, lowered Priority and Severity, 
changed Summary to "nsStdUrl->Equals() doesn't work with nsMsgMailNewsUrl" from 
"Crash opening mail message".

Reassigned to mscott to fix the inheritance problem in nsMsgMailNewsUrl.
Scott: when you're done, please remove my patch in nsCSSLoader.cpp.

Comment 41

[laurel]

The mailnews crash part of this (bug 46293) appears to be fixed using the oct10
mn6 branch commercial build, linux rh6.0, mac OS 9.0 and NT 4.0

Comment 42

[Pierre Saslawsky]

Reset the target milestone to put this bug back on mscott's radar.

Comment 43

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 44

[Darin Fisher]

bienvenu's patch for bug 98626 (applied 2002/09/25) might also fix this bug,
making the CSS loader workaround/hack unnecessary.  can someone confirm?

Comment 45

[David :Bienvenu]

I'm pretty sure you're right - I did fix Equals to work for mailnews urls, and
what the style sheet code is doing is very similar to what the imglib code was
doing that prompted me to fix Equals. Do you mind if I mark this one fixed/dup,
or do you want to leave it open to see about changing the css loader?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Either way.  If you mark it a dup, please file a bug on me for the CSSLoader
change.  I'd like to land bug 107567 before making that change.  ;)

Comment 47

[David :Bienvenu]

bug 182248 filed to remove workaround.

*** This bug has been marked as a duplicate of 98626 ***