148233 - [acrobat] clicking on weblink inside PDF via mail attachments crashes in NPN_GetURL

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Peter Lubczynski]

Steps to repro:

1. E-mail to yourself the PDF in this bug as an attachment
2 [details] [diff] [review]. Open the message with the PDF and double click to view attachment
3 [details] [diff] [review]. Click on web link inside PDF
4. Crash!

Stack:
NS_MakeAbsoluteURI [nsNetUtil.h, line 253]
pluginInstanceOwner::GetURL [nsPluginViewer.cpp, line 1159]     
nsPluginHostImpl::GetURLWithHeaders [nsPluginHostImpl.cpp, line 2921]     
nsPluginHostImpl::GetURL [nsPluginHostImpl.cpp, line 2870]     
MakeNew4xStreamInternal [ns4xPlugin.cpp, line 792]   
_geturl [ns4xPlugin.cpp, line 807]      
nppdf32.dll + 0x4a27 (0x0bb54a27)  

Cause:
It appears that the IMAP channel doesn't have its URL set. When doing
NPN_GetURL, we call into the channel to get the URL. This happens to return NULL
but we don't check and crash when attempting to figure out the absolute URL.

Solution:
Since the IMAP channel is not giving the correct URL, so lets instead get the
document's URL directly from the document (which is correct and set from the
channel when we are instantiated). I also added a warning into GetURI just in
case we hit this problem again elsehwere.

Comment 1

[Peter Lubczynski]

Attachment: testcase: PDF with web link

Comment 2

[Peter Lubczynski]

Attachment: patch v.1

possible fix?

Comment 3

[rubydoo123]

1.1 for now

Comment 4

[av (gone)]

Comment on attachment 85659 [details] [diff] [review]
patch v.1

r=av

Comment 5

[serge (gone)]

hmm, the strange thing is I cannot reproduce the crash on 05.26 debug trunk build:(

Comment 6

[serge (gone)]

and Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.0) Gecko/20020528
Netscape/7.0b1 also WFM

Comment 7

[serge (gone)]

but 
inline nsresult
NS_MakeAbsoluteURI(nsACString &result,
                   const nsACString &spec, 
                   nsIURI *baseURI = nsnull, 
                   nsIIOService *ioService = nsnull)     // pass in nsIIOService
to optimize callers
{
    NS_ASSERTION(baseURI, "It doesn't make sense to not supply a base URI");
    if (spec.IsEmpty())
        return baseURI->GetSpec(result);
    return baseURI->Resolve(spec, result);
}
does not look good, you can call it as NS_MakeAbsoluteURI(resultBuf, spec);
and crash righ away, cuz nsIURI *baseURI == nsnull:(

Comment 8

[Peter Lubczynski]

Attachment: patch to NS_MakeAbsoluteURI

Serge is right, if some joker calls NS_MakeAbsoluteURI(resultBuf, spec), we're
dead. This patch makes |NS_MakeAbsoluteURI| return a copy of the string passed
in if the baseURI is missing.

Comment 9

[Darin Fisher]

looks like you guys discovered bug 67619 :(

Comment 10

[Darin Fisher]

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

Comment 11

[Peter Lubczynski]

can I get reviews?

Comment 12

[Darin Fisher]

Comment on attachment 86345 [details] [diff] [review]
patch to NS_MakeAbsoluteURI

nit:

can you eliminate the nsnull default value for baseURI?

and can you please indent by 4 spaces?

with those 2 changes, sr=darin

Comment 13

[Peter Lubczynski]

Attachment: patch v.3

This patch addresses Darin's comments plus also does the same thing to the
overloaded function.

Comment 14

[Darin Fisher]

Comment on attachment 87590 [details] [diff] [review]
patch v.3

thx for catching the other function!!  sr=darin

Comment 15

[serge (gone)]

Comment on attachment 87590 [details] [diff] [review]
patch v.3

yeah, it looks much better, r=serge

Comment 16

[Peter Lubczynski]

oops, patch landed on trunk on 6/14, forgot to mark this FIXED.

Because this is a plugin crasher, I'm nominating for 1.0.

Comment 17

[Jaime Rodriguez, Jr.]

shrir - can you pls verify this fix on the trunk? thanks!

Comment 18

[shrirang khanzode]

verified on 0617 trunk NT..no crash for me. Looks good.

Comment 19

[Jaime Rodriguez, Jr.]

adt1.0.1 (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then add the "fixed1.0.1" keyword.

Comment 20

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

Comment on attachment 87590 [details] [diff] [review]
patch v.3

Please land this on the 1.0.1 branch.  Once there, replace the
"mozilla1.0.1+" keyword with the "fixed1.0.1" keyword.


(I wonder, though, for the future (on the trunk) whether it's bad for code size
(since these are inline functions) to have the null check even for callers that
don't need it.)

Comment 21

[Peter Lubczynski]

fixed1.0.1, landed on 1.0 branch

Comment 22

[Terri Preston]

Verified fixed win XP branch build 2002062508