393309 - Crash @nsInstallTrigger::HandleContent when browsing directly to XPI file

Status: VERIFIED FIXED

Description

[cmtalbert]

Minefield is crashing when I attempt to browse to a non-AMO site that contains an extension.  This seems to happen even after I add the site to the whitelist.

BuildID: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007082204 Minefield/3.0a8pre

I have two crash logs:
http://crash-stats.mozilla.com/report/index/67822257-50f4-11dc-b579-001a4bd46e84?date=2007-08-22-21

http://crash-stats.mozilla.com/report/index/f6b8a205-50f8-11dc-8919-001a4bd43ed6?date=2007-08-22-21

== Steps ==
1. Browse to https://landfill.bugzilla.org/mozillaQA/latest.xpi

== Actual Results ==
Crash

== Expected Results ==
Get the warning box stating something like "do you want to save this or install it into firefox.  Note this site is not trusted etc etc".

Note that you can go to https://landfill.bugzilla.org/mozillaQA/ and install the XPI from there, and white list the site.  Then when you try directly linking to the XPI again, it will crash on you once more.

I'm putting this in extension compatibility because we're in nsInstallInfo in the stack, but it might actually belong in the Download Manager component.

I was also able to confirm this crash with the same nightly Aug 22, 2007 on Windows XP: http://crash-stats.mozilla.com/report/index/11109a1c-50fe-11dc-b23f-001a4bd43ef6?date=2007-08-22-22

Since this is a crash on two OS's (and probably on linux too) --> status Critical.

Comment 1

[Dave Townsend [:mossop]]

Caused by bug 307770, will be fixed by the patch in bug 252830.

On another note that breakpad stack is rubbish, it links to the wrong file for the given method.

Comment 2

[Jürgen Herz]

Is this the same reason FF trunk and SM trunk newer than 20070819 crash when dragging any XPI file from the desktop on a browser window (tested on Windows)?

On crash I get a box saying
"Unfortunately the crash reporter is unable to submit a crash report.
The application did not leave a crash dump file."
So no report, sorry.

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #2)
> Is this the same reason FF trunk and SM trunk newer than 20070819 crash when
> dragging any XPI file from the desktop on a browser window (tested on Windows)?

Yes I think that's pretty likely the same.

Comment 6

[(not reading, please use [e-mail] instead)]

at least for bug #393682, we are crashing in nsInstallTrigger::HandleContent(), dereferencing referringURI when it is null

Here's a stack to the crash:

>	xpinstal.dll!nsInstallTrigger::HandleContent(const char * aContentType=0x0d086f90, nsIInterfaceRequestor * aWindowContext=0x29aefe28, nsIRequest * aRequest=0x0d086f00)  Line 212 + 0x25 bytes	C++
        docshell.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest *
request=0x0d086f00, nsISupports * aCtxt=0x00000000)  Line 500 + 0x47 bytes     
  C++
        docshell.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest *
request=0x0d086f00, nsISupports * aCtxt=0x00000000)  Line 280 + 0x10 bytes C++
        necko.dll!nsBaseChannel::OnStartRequest(nsIRequest *
request=0x0a6ac040, nsISupports * ctxt=0x00000000)  Line 604 + 0x46 bytes  C++
        necko.dll!nsInputStreamPump::OnStateStart()  Line 439 + 0x2c bytes     
C++
        necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *
stream=0x09c0d0c0)  Line 395 + 0xb bytes  C++
        xpcom_core.dll!nsInputStreamReadyEvent::Run()  Line 112 C++
        xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int *
result=0x0012fa00)  Line 491     C++
        xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bce7e8, int
mayWait=1)  Line 227 + 0x16 bytes       C++
        gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes        C++
        tkitcmps.dll!nsAppStartup::Run()  Line 170 + 0x1c bytes C++
        xul.dll!XRE_main(int argc=3, char * * argv=0x00bc9660, const
nsXREAppData * aAppData=0x00bc9a50)  Line 3069 + 0x25 bytes        C++
        firefox.exe!main(int argc=3, char * * argv=0x00bc9660)  Line 153 + 0x12
bytes   C++
        firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes  C
        firefox.exe!mainCRTStartup()  Line 403  C
        kernel32.dll!7c816fd7()         
        [Frames below may be incorrect and/or missing, no symbols loaded for
kernel32.dll]

working on a potential fix.

Comment 7

[(not reading, please use [e-mail] instead)]

Created attachment 278202 [details] [diff] [review]
patch

the comment in the code already points to the issue.

from http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsInstallTrigger.cpp#164

164         // It's possible docshell explicitly set a null referrer in the case
165         // of typed, pasted, or bookmarked URLs and the like. In such a case
166         // we get a success return value with null pointer.

Comment 8

[Dave Townsend [:mossop]]

That patch will change the whitelist check to use the xpi url in the event of no referer, thus requiring it to pass the whitelist even for manually entered urls and such which is not what we want I think.

The patch in bug 252830 will fix this. I'm just wondering though whether it might just be sensible to back out bug 307770 until then. This is showing up as the 8th most popular crasher on breakpad.

Comment 9

[(not reading, please use [e-mail] instead)]

> That patch will change the whitelist check to use the xpi url in the event of
> no referer, thus requiring it to pass the whitelist even for manually entered
> urls and such which is not what we want I think.

good point, comparing to fx 2, this is not what we want.  

since bug #252830 will fix this, should we just wait for that fix?

re-assigning to dave, as he has the fix in his other bug.

Comment 10

[(not reading, please use [e-mail] instead)]

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

obsoleting, based on dave's comment.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Created attachment 278424 [details] [diff] [review]
fix the crash

This avoids crashing by null checking the URI before calling GetHost. If referringURI is null, we don't need the host anyways, because the only case where a load with a null referringURI is disallowed is if the global xpinstall.disabled pref is false, in which case the UI will only show the "xpinstall disabled, enable it?" notification rather than the "add to whitelist?" notification.

This will get reworked in bug 252830, but I think we should just fix this crasher for now until that patch can land.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(assume that dangling bracket is removed :)

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 278424 [details] [diff] [review]
fix the crash

Looks good. Will be worth watching breakpad to see if the other case really is happening.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

mozilla/xpinstall/src/nsInstallTrigger.cpp 	1.80

Comment 16

[cmtalbert]

Verified this fix with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007082904 Minefield/3.0a8pre

Nice work.  Also added litmus testcase 4598 (http://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=4598) to catch this regression.

--> VERIFIED

Comment 17

[Dave Townsend [:mossop]]

Breakpad is currently showing 0 crashes for this from todays build so it looks like those stacks pointing to uri are probably faulty.