Last Comment Bug 752877 - Cut wrappers after firing [inner,outer]-window-destroyed.
: Cut wrappers after firing [inner,outer]-window-destroyed.
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 3 votes (vote)
: mozilla15
Assigned To: Kyle Huey [:khuey] (
Depends on: hueyfix 751420 753621
  Show dependency treegraph
Reported: 2012-05-08 05:56 PDT by Kyle Huey [:khuey] (
Modified: 2013-12-27 14:22 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (4.13 KB, patch)
2012-05-08 05:56 PDT, Kyle Huey [:khuey] (
bzbarsky: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] ( 2012-05-08 05:56:05 PDT
Created attachment 621945 [details] [diff] [review]
Comment 1 Manoj 2012-05-08 14:47:05 PDT
Thanks for filing the follow-up bug.
Comment 2 Boris Zbarsky [:bz] 2012-05-08 19:07:16 PDT
Comment on attachment 621945 [details] [diff] [review]

Comment 3 Kyle Huey [:khuey] ( 2012-05-09 08:12:51 PDT
Comment 4 Nicholas Nethercote [:njn] 2012-05-09 16:46:59 PDT
My tiny brain cannot handle this bug's pithiness.  Which problem does it address?
Comment 5 Boris Zbarsky [:bz] 2012-05-09 16:51:28 PDT
Before this fix we'd post an async event to the event loop, then cut the wrappers from chrome to content.  When the event fired some time later, we'd send a notification to chrome that the window is going away.  Some chrome (including old addon SDK versions) would then try to run cleanup code that touched the window, and this code would throw because the wrappers had been neutered.

This fix just moves the neutering of the wrappers to run right after the notification, from the async event.
Comment 6 Nicholas Nethercote [:njn] 2012-05-09 16:59:21 PDT
Thanks!  So bug 751466 is less urgent now, good.
Comment 7 Boris Zbarsky [:bz] 2012-05-09 17:02:12 PDT
That was the idea, yes.  ;)
Comment 8 Boris Zbarsky [:bz] 2012-05-09 17:02:40 PDT
But we should perhaps put that information in that bug.  Kyle, want to do that, since you did the actual testing here?
Comment 9 Kyle Huey [:khuey] ( 2012-05-09 17:05:06 PDT
I think I already did that in comment 31 ...
Comment 10 Michael Foerster 2012-05-15 03:38:13 PDT
Any chance to to get this solution (and the under-laying bug 695480) elevated to Aurora (mozilla14) ?

With this fix, the win in term of memory and leaks is so much greater than the hassle of (some) breaking Add-ons.

Please correct me if I'm wrong.
Comment 11 Olli Pettay [:smaug] 2012-05-15 03:47:05 PDT
Bug 695480 (and this one as a followup) is quite risky change, so getting it to Aurora
isn't likely.
Comment 12 Justin Lebar (not reading bugmail) 2012-05-15 07:18:53 PDT
> Any chance to to get this solution (and the under-laying bug 695480) elevated to Aurora (mozilla14) ?

Lots of bugfixes landed after bug 695480 -- see the list of dependent bugs.  So there's really no way we're going to port this up to Aurora.
Comment 13 Nicholas Nethercote [:njn] 2012-05-15 17:33:12 PDT
And bug 695480 is a big enough change that we want a full development cycle to shake out any problems it causes.  (This very bug is a perfect example of the kind of problem we need to shake out.)

Note You need to log in before you can comment on or make changes to this bug.