292362 - Crash on restart with extension installed [@ CarbonCore.557.0.0]

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Tracy Walker [:tracy]]

from crot0 bug 291666 https://bugzilla.mozilla.org/show_bug.cgi?id=291666#c34

Created an attachment (id=182168)
https://bugzilla.mozilla.org/attachment.cgi?id=182168
crash log

In the Mac version, it crashes when Extension is installed and Firefox is
re-start. 
And Extension is not installed though it doesn't crash when start is done
again. 
(In Extention Manager, it is a message "When being start next time, it is
installed". )

I used Flashgot to test this extension installation.  see URL field

Comment 1

[Chase Phillips]

FYI this build is from atlantia, a new machine.  Our goal is to have atlantia
replace cadence as our Firefox Mac trunk build system.  Did you see this problem
in yesterday's build?

I can reconfigure cadence to do a one-off build this afternoon which would be
useful to see if there was some code change in the past 24 hours that's causing
this problem or if some difference between the build systems is causing it.

Comment 2

[Robert Strong (they/them - no direct email)]

Chase - prior to today's builds an extension's files were not being extracted
which was fixed by bug 291666 so it is possible that either the change in
machines or the patch caused this bug. I will have a Mac OS X system this
evening to do some testing but the amount of time I have access to it will be
limited so having a one-off build available would be greatly appreciated.

Comment 3

[Hiro]

(In reply to comment #1)
> I can reconfigure cadence to do a one-off build this afternoon which would be
> useful to see if there was some code change in the past 24 hours that's causing
> this problem or if some difference between the build systems is causing it.

The same crash error occurred in Firefox that did Tinderbox build and for myself
build. 
And Latest nightly build is generated. 

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050429
Firefox/1.0+

Incident ID: TB5461743W

Comment 4

[Robert Strong (they/them - no direct email)]

Looking at the talkback I see that nsDirEnumerator::HasMoreElements() is towards
the top. There was a patch for bug 291177 checked in that modified this
interface at the same time the new EM code was checked in. I'm not stating that
bug 291666 didn't cause this but I highly suspect that bug 291177 is the cause
due to:
The talkback specifically states this interface towards the top of the stack
immediately below CarbonCore.
The patch for bug 291666 creates directories / files and extracts files using
methods that have been in use for quite some time and haven't changed recently.

I don't have a development environment on the Mac system I will be borrowing
tonight but the patch for bug 291666 is just JavaScript so I will be able to
check if it is in fact causing this crash or if it is just allowing other code
to execute that causes this crash.

Comment 5

[Robert Strong (they/them - no direct email)]

Well, I was half right. This wasn't caused by bug 291666 or by
nsDirEnumerator::HasMoreElements(). It turns out that it is caused by an entry
in the new extensions-startup.manifest file that was introduced by bug 286034.
The entry is for the default theme and the suspicious field is for the path
which is a persistent descriptor (e.g. the full path to the theme). One way to
verify or work around this after experiencing this bug is to open the
extensions-startup.manifest file in the profile directory, delete the entire
line that starts with app-global {972ce4c6-7e08-4474-a285-3208198ce6fd}, and
then restart. For me Firefox starts right up and the extensions worked.

Comment 6

[Robert Strong (they/them - no direct email)]

Someone that is more familiar with Mac file descriptors had better take this
on... I'll keep looking but I don't know enough about them or this new section
of code to own this bug.

Comment 7

[Robert Strong (they/them - no direct email)]

After debugging some more I found I found that I could disabled the ext -
restart, enable the ext - restart and the extension would be functional. Also,
the profile's extensions.rdf file is not created and the staged-xpis directory
is always left behind after an install.

I then found that commenting out the following two lines for file removal fixed
the crash condition on install so it appears not to be
extensions-startup.manifest related.
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#3421
    if (file)
      installLocation.removeFile(file);

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch

It appears that a call to nsIDirectoryEnumerator hasMoreElements after the
directory has been deleted causes a crash on Mac OS X which probably should be
fixed in another bug. There is also a fix for an extra param to a function that
doesn't take an extra param. This patch has been tested on Mac OS X, Windows,
and Linux.

Note: There is still several other issues with the EM at this point that this
patch doesn't fix but this patch appears to bring Mac OS X to the same level of
functionality as other OS's in regards to the new EM code.

Comment 9

[Jay Patel [:jay]]

Adding Talkback info and topcrash+ keyword. According the the latest Talkback data:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=CarbonCore.557.0.0&vendor=MozillaOrg&product=FirefoxTrunk&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

This is a regression that started on 4/26.

Comment 10

[Jay Patel [:jay]]

Adding Talkback info and topcrash+ keyword. According the the latest Talkback data:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=CarbonCore.557.0.0&vendor=MozillaOrg&product=FirefoxTrunk&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

This is a regression that started on 4/26.

Comment 11

[Jay Patel [:jay]]

Sorry accidently reassigned to default...returning to proper owner.

Comment 12

[Robert Strong (they/them - no direct email)]

BTW: the patch in bug 292506 includes this fix. I was hoping for either this
simple patch to get reviewed quickly and checked in or if someone had the time
to review a larger patch covering a bunch of simple patches grouped together
(e.g. called function names incorrect, wrong param order, etc.) that the patch
in bug 292506 would be reviewed and checked in.

Comment 13

[Mike Connor [:mconnor]]

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

>         parent.remove(false);
>         parent = parent.parent;
>+        entries = parent.directoryEntries;
>       }
>       if (entries instanceof nsIDirectoryEnumerator)
>         entries.close();
>     }
>     catch (e) {
>+      LOG("DirectoryInstallLocation::removeFile: failed to remove staged " + 
>+          " directory = " + parent.path + ", exception = " + e + "\n");

is this just wallpaper that should be removed once the real bug is fixed? if
we're crashing on Mac where we don't elsewhere, there should be a bug on the
real problem, and a comment here noting the wallpaper and the bug number

Without knowing the EM code in-depth, I think this seems like the right bit of
code anyway.

Comment 14

[Mike Connor [:mconnor]]

We need this for alpha1

Comment 15

[Robert Strong (they/them - no direct email)]

It isn't wallpaper and should not be removed... as a matter of fact this
function was not doing what was expected on all OS's when hasMoreElements()
would have returned true which this patch fixes. The code in question is within
a loop and !entries.hasMoreElements() loses its reference immediately following
the parent.remove(false) so the second time entries is no longer valid which is
where Mac OS X crashed. Ben checked in a patch for bug 291177 that added support
for scripting nsDirEnumerator and it appears that in all cases except for Mac OS
X it doesn't crash when the reference is no longer valid. I'll submit a separate
bug for the issue issue with nsIDirectoryEnumerator.

>       while (parent && !parent.equals(this.location) &&
>             !entries.hasMoreElements()) {
>         parent.remove(false);
>         parent = parent.parent;
>+        entries = parent.directoryEntries;
>       }
>       if (entries instanceof nsIDirectoryEnumerator)
>         entries.close();
>     }
>     catch (e) {
>+      LOG("DirectoryInstallLocation::removeFile: failed to remove staged " + 
>+          " directory = " + parent.path + ", exception = " + e + "\n");

Comment 16

[Robert Strong (they/them - no direct email)]

I opened Bug 292823 to fix the nsIDirectoryEnumerator interface where the root
cause of the crash resides.

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch

mconnor - I added the comment at the section where this bug occurs as well as
where the constant for nsIDirectoryEnumerator is declared. This could happen
anywhere in the code that uses nsIDirectoryEnumerator and there are several of
them so this is safer. I referenced bug 292823 since that is where the root
cause of this issue will be fixed.

Comment 18

[Asa Dotzler [:asa]]

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

a=asa

Comment 19

[Robert Strong (they/them - no direct email)]

patch checked in - thanks db48x. marking resolved fixed