Closed Bug 482687 Opened 11 years ago Closed 11 years ago

Crash when closing browser while customize toolbar dialog is open [@ nsXULWindow::SavePersistentAttributes() ]

Categories

(Core :: XUL, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: smaug, Assigned: zwol)

References

()

Details

(Keywords: crash, topcrash, verified1.9.1)

Crash Data

Attachments

(2 files)

so we're crashing on line 1468 of nsXULWindow.cpp, which is in nsXULWindow::SavePersistentAttributes()...

   PRInt32 appPerDev = mWindow->GetDeviceContext()->AppUnitsPerDevPixel();

and that's being called from nsWebShellWindow::Destroy() during process teardown.  I presume the problem is that at this point there's no device context anymore, or else no mWindow (which is a nsIWidget) at all.  (It would be nice if crash reports included a small disassembly dump around the faulting instruction, and the register contents.)

I don't know how to fix it, though.  SavePersistentAttributes really does need the app-units-per-dev-pixel conversion factor and has nowhere to get it but the device context.  It could bail out if mWindow or mWindow->GetDeviceContext() is null, but then I think we wouldn't save the browser window position at all in normal shutdown.  (Some people might see this as a feature. ;-)  Or we could find an earlier point in global teardown to call SavePersistentAttributes, and not do it from nsWebShellWindow::Destroy().  I'm not familiar enough with this code to say where that might be.
mWindow is cleared in nsXULWindow::Destroy(). Could you perhaps cache
appPerDev there?
Or maybe nsWebShellWindow should call nsXULWindow::SavePersistentAttributes()
before nsXULWindow::Destroy()
(In reply to comment #3)
> Or maybe nsWebShellWindow should call nsXULWindow::SavePersistentAttributes()
> before nsXULWindow::Destroy()

It does!
So mWindow is cleared earlier?
Either that or nsIWidget::GetDeviceContext() is returning null.  Unfortunately the crash report doesn't have any frames prior to nsWebShellWindow::Destroy for some reason.
Duplicate of this bug: 483376
I think we should paper over this by saving appunits-per-devpixel in the window and using it if mWindow or GetDeviceContext() returns null. We should also have an assertion that fires if those are null, to help us track down why that might be happening.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Assignee: nobody → zweinberg
Attached patch candidate fixSplinter Review
Ok, here's a patch following that suggestion.  Works for me on linux (but I don't see the crash in the first place, & the assertion in SavePersistentAttributes does not fire) & pushed to the try server, will follow up.
Attachment #367792 - Flags: review?(roc)
I think this would break if we changed the DPI dynamically, because we're not ever resetting mAppPerDev.

I think we should actually have a method to get the appPerDev value that always tries the window first and if it's there, sets mAppToDev before returning. Otherwise just returns mAppToDev. Make sense?
Sure.  Do I have to bump the IID for a new protected method?
OK, here's an alternative patch with a wrapper method for mAppPerDev.  This winds up touching less code which is nice right now.  I've not put this on the try server, though, because it seems to have a nasty backlog and the previous patch should do to confirm the fix.
Attachment #367817 - Flags: review?(roc)
Comment on attachment 367817 [details] [diff] [review]
alternative approach with nsXULWindow::AppUnitsPerDevPixel() method

looks great
I'd like confirmation that this fixes the bug before requesting commit ... Smaug, can you reproduce the bug?  If so, do you have time to try the tryserver build when it's done?
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/60da3e72b4cc
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090326 Minefield/3.6a1pre ID:20090326050203

This crash happened when you have open the customize toolbar dialog and close the browser window while the dialog is open. With the todays trunk nightly build the crash doesn't occur anymore.

No more crashes listed on 1.9.2 since the check-in.
Status: RESOLVED → VERIFIED
Hardware: x86 → All
Summary: Crash [@ nsXULWindow::SavePersistentAttributes() ] → Crash when closing browser while customize toolbar dialog is open [@ nsXULWindow::SavePersistentAttributes() ]
Target Milestone: --- → mozilla1.9.2a1
tagging checkin-needed for 1.9.1 branch landing.
Keywords: checkin-needed
Verified fixed on 1.9.1 with nativeMozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090410 Shiretoko/3.5b4pre (.NET CLR 3.5.30729) ID:20090410053901

Last crash reported to Socorro is with build id 20090409043738.
Severity: normal → critical
Keywords: crash
Crash Signature: [@ nsXULWindow::SavePersistentAttributes() ]
You need to log in before you can comment on or make changes to this bug.