Faviconize Tab extension causes a crash @ nsNavHistory::LazyMessage::Init

VERIFIED FIXED in Firefox 3 beta2

Status

()

Firefox
Bookmarks & History
P3
normal
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: thunder, Assigned: thunder)

Tracking

({crash})

Trunk
Firefox 3 beta2
x86
All
crash
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
Created attachment 289547 [details]
Stack trace

In recent builds, the Faviconize Tab extension causes a crash (see attached trace).
(Assignee)

Comment 1

10 years ago
Created attachment 289552 [details] [diff] [review]
Fix v1

Faviconize tab is setting an icon and places code isn't properly checking the URI it sends.  This patch adds a check, which fixes the crash.
Assignee: nobody → thunder
Status: NEW → ASSIGNED
Attachment #289552 - Flags: review?(sspitzer)
can you figure out what we're calling setAndLoadFaviconForPage() with?

http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#501
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M10
Comment on attachment 289552 [details] [diff] [review]
Fix v1

r=sspitzer, I'd like to get this into m10
Attachment #289552 - Flags: review?(sspitzer)
Attachment #289552 - Flags: review+
Attachment #289552 - Flags: approval1.9?
Summary: Faviconize Tab extension blows up → Faviconize Tab extension causes a crash @ nsNavHistory::LazyMessage::Init
(Assignee)

Comment 4

10 years ago
(In reply to comment #2)
> can you figure out what we're calling setAndLoadFaviconForPage() with?
> 
> http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#501

I added a dump() there, and here's what I see on stdout now:

aURI: [xpconnect wrapped nsIURI @ 0x180e4910 (native @ 0x180e47b4)]
WARNING: NS_ENSURE_TRUE(aURI) failed: file /Users/thunder/sources/mozilla-trunk/mozilla/toolkit/components/places/src/nsNavHistory.h, line 444

So it's sending an nsIURI object, but it is invalid somehow.  Maybe due to Init() being lazy, the object is being collected before Init() gets run?  Just a guess.

Updated

10 years ago
Attachment #289552 - Flags: approval1.9? → approval1.9+
this just showed up on crash-stats, on linux and windows for build id 2007112000

before that, nothing.

In addition to checking in, I think we still need to figure out what's going on.

Thunder, can you still reproduce at will?
Keywords: crash

Comment 6

10 years ago
Multiple extensions are being bit by bug 404499.  And the first nightly build with that problem was 11/20.  So perhaps that's the culprit.  

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> Multiple extensions are being bit by bug 404499.  And the first nightly build
> with that problem was 11/20.  So perhaps that's the culprit.  

Perhaps.  But I think we should still check in setAndLoadFaviconForPage() that a URI gets passed in.

I committed the patch:

Checking in nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.113; previous revision: 1.112
done
here's what's going on:

short version:  this add on is not compatible with fx 3 for good reason, as it makes assumptions about the setIcon() method in tabbrowser.xml

long version:  In order to use this add on, I had to set extensions.checkCompatibility to true.

in content/addon/auto_fav.js of this addon, the author has code that dynamically changes the setIcon() method.

in fx2, that method was:

http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/tabbrowser.xml#450

In fx 3, it now looks like this:

http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#487

When the add on code munges setIcon, we end up with:

gBrowser.setIcon = function setIcon(aTab, aURI) {
    !aURI && faviconize.autoFav.loaded(aTab);var browser = this.getBrowserForTab(aTab);
    browser.mIconURL = aURI;
    if (aURI) {
        if (!(aURI instanceof Components.interfaces.nsIURI)) {
            var ios = Components.classes['@mozilla.org/network/io-service;1'].getService(Components.interfaces.nsIIOService);
            aURI = ios.newURI(aURI, null, null);
        }
        this.mFaviconService.setAndLoadFaviconForPage(browser.browser, aURI, false);
    }
    this.updateIcon(aTab);
    for (var i = 0; i < this.mProgressListeners.length; i++) {
        var p = this.mProgressListeners[i];
        if ("onLinkIconAvailable" in p) {
            p.onLinkIconAvailable(browser);
        }
    }
}

notice:

this.mFaviconService.setAndLoadFaviconForPage(browser.browser, aURI, false);

browser.browser is null, so we are calling setAndLoadFaviconForPage() with null, which eventually makes its way down to nsNavHistory::LazyMessage::Init(), and then we crash.

Dan wrote:

"But I think we should still check in setAndLoadFaviconForPage() that a URI gets passed in."

I agree, because passing in setAndLoadFaviconForPage() will crash firefox.

marking as fixed on behalf of Dan, but I'm going to attach a supplemental patch and test case for the favicon service.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Created attachment 289750 [details] [diff] [review]
supplimental patch, check args and test case to ensure we throw on null args

make sure if someone calls us with null args, we don't crash.

for the second null arg, we'd crash in AddLazyLoadFaviconMessage() when we dereferenced the null aFavicon:

  rv = aFavicon->Clone(getter_AddRefs(message.favicon));
Attachment #289750 - Flags: review?(thunder)
(Assignee)

Comment 10

10 years ago
Comment on attachment 289750 [details] [diff] [review]
supplimental patch, check args and test case to ensure we throw on null args

Thanks Seth!
Attachment #289750 - Flags: review?(thunder) → review+
supplemental fix landed:

Checking in src/nsFaviconService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsFaviconService.cpp,v  <--  nsFaviconService.cpp
new revision: 1.17; previous revision: 1.16
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_404630.js,v
done
Checking in tests/unit/test_404630.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_404630.js,v  <--  test_404630.js
initial revision: 1.1
done

I've also verified that thunder's original fix fixed the crasher with the add-on.
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.