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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase (must be loaded as *mailbox* in mail)

Comment 2

[Boris Zbarsky [:bzbarsky]]

So how exactly do I go about reproducing with that testcase?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This fixes the bug by propagating errors, and also saves an extra
QueryInterface.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[David :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?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Not that ExtractScheme returns any success result other than NS_OK...)

Comment 10

[Scott MacGregor]

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.

Comment 11

[David :Bienvenu]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 146725 [details] [diff] [review]
patch

Probably worth getting this in for the next RC of 1.7.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2004-04-21 16:00 -0700.

Comment 14

[chris hofmann]

Comment on attachment 146725 [details] [diff] [review]
patch

a=chofmann for 1.7

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_7_BRANCH, 2004-04-21 20:09 -0700.