Closed Bug 393309 Opened 12 years ago Closed 12 years ago

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

Categories

(Core Graveyard :: Installer: XPInstall Engine, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9alpha8

People

(Reporter: cmtalbert, Assigned: Gavin)

References

Details

(Keywords: crash, regression)

Attachments

(1 file, 1 obsolete file)

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.
Summary: Crash @nsInstallTrigger::HandleContent when linking directly to XPI file → Crash @nsInstallTrigger::HandleContent when browsing directly to XPI file
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.
Blocks: 307770
Component: Extension Compatibility → Installer: XPInstall Engine
Depends on: 252830
Keywords: crash, regression
Product: Firefox → Core
QA Contact: extension.compatibility → xpi-engine
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.
(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.
Duplicate of this bug: 393394
Duplicate of this bug: 393682
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #278202 - Flags: review?(dveditz)
Assignee: nobody → sspitzer
Flags: in-litmus?
Target Milestone: --- → mozilla1.9 M8
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.
> 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.
Assignee: sspitzer → dtownsend
Comment on attachment 278202 [details] [diff] [review]
patch

obsoleting, based on dave's comment.
Attachment #278202 - Attachment is obsolete: true
Attachment #278202 - Flags: review?(dveditz)
Attached patch fix the crashSplinter Review
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.
Assignee: dtownsend → gavin.sharp
Status: NEW → ASSIGNED
Attachment #278424 - Flags: review?
Attachment #278424 - Flags: review? → review?(dtownsend)
(assume that dangling bracket is removed :)
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.
Attachment #278424 - Flags: review?(dtownsend) → review+
Duplicate of this bug: 393708
Flags: blocking1.9+
mozilla/xpinstall/src/nsInstallTrigger.cpp 	1.80
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Breakpad is currently showing 0 crashes for this from todays build so it looks like those stacks pointing to uri are probably faulty.
Duplicate of this bug: 394254
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.