Closed Bug 241254 Opened 20 years ago Closed 20 years ago

crash on message w/ HTML attachment that document.writes LINK rel=stylesheet with relative URL

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: dbaron)

Details

(Keywords: crash, fixed1.7)

Attachments

(2 files)

This is spun off of bug 241128.

We crash when a forwarded HTML file without a Content-Base or BASE HREF
document.writes a LINK rel=stylesheet with a relative URL.  See bug 241128
comment 7 and bug 241128 comment 21.
Attachment #146720 - Attachment description: testcase → testcase (must be loaded as *mailbox* in mail)
Attachment #146720 - Attachment mime type: message/rfc822 → text/plain
So how exactly do I go about reproducing with that testcase?
Attached patch patchSplinter Review
This fixes the bug by propagating errors, and also saves an extra
QueryInterface.
(Sorry, I didn't figure out until after I attached the testcase that it has to
be used as a mailbox in mailnews, not as a standalone message.)
Fun stuff.....

I thought a bit more, and I have a good idea why the crash is happening in
general.  It could perhaps be fixed for "foo:bar" uris too, but I think I'd
rather try to just fix bug 84582.
Comment on attachment 146725 [details] [diff] [review]
patch

I'm really not qualified to assess the risk of this, but it does seem to me
like the right thing here.
Attachment #146725 - Flags: superreview?(mscott)
Attachment #146725 - Flags: review?(bienvenu)
     if (NS_SUCCEEDED(rv) && !scheme.IsEmpty())
     {
       result = relativePath;
+      rv = NS_OK;
     }

if NS_SUCCEEDED(rv), do we really need to set it to NS_OK? If so, that would
seem to be a problem in the calling code treating NS_OK specially. Or am I
missing something?
Well, I was just thinking we might not want to propagate any old success value,
but propagating it should be fine as well, as long as you think it makes sense
to propagate the exact result of ExtractScheme rather than just whether there
was a scheme.
(Not that ExtractScheme returns any success result other than NS_OK...)
Comment on attachment 146725 [details] [diff] [review]
patch

dbaron to the rescue.

This looks like the right kind of error propogation. David and I can do risk
assessment on the M4 branch. But for the trunk and possibly 1.7 consideration,
seems good for both to me.
Attachment #146725 - Flags: superreview?(mscott) → superreview+
Comment on attachment 146725 [details] [diff] [review]
patch

that's fine - I just was worried that some caller was doing the wrong thing.
this does fix the problem for me, and re the risk factor, I'm not too worried
about propagating the error.
Attachment #146725 - Flags: superreview?(mscott)
Attachment #146725 - Flags: superreview+
Attachment #146725 - Flags: review?(bienvenu)
Attachment #146725 - Flags: review+
Attachment #146725 - Flags: superreview?(mscott) → superreview+
Comment on attachment 146725 [details] [diff] [review]
patch

Probably worth getting this in for the next RC of 1.7.
Attachment #146725 - Flags: approval1.7?
Fix checked in to trunk, 2004-04-21 16:00 -0700.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment on attachment 146725 [details] [diff] [review]
patch

a=chofmann for 1.7
Attachment #146725 - Flags: approval1.7? → approval1.7+
Fix checked in to MOZILLA_1_7_BRANCH, 2004-04-21 20:09 -0700.
Keywords: fixed1.7
Severity: normal → critical
Keywords: crash
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: