Last Comment Bug 725210 - Add internal observer notification for last PB docshell going away
: Add internal observer notification for last PB docshell going away
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: PBnGen 722845 722850 722857 722859 722861 722868 722979 722981 730840
  Show dependency treegraph
 
Reported: 2012-02-07 22:20 PST by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-04-26 02:03 PDT (History)
6 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
<required> (2.99 KB, patch)
2012-02-07 22:20 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Add observer notification for last PB docshell going away. (3.01 KB, patch)
2012-02-07 22:22 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
bzbarsky: review+
Details | Diff | Splinter Review
Add observer notification for last PB docshell going away. (3.19 KB, patch)
2012-02-21 21:29 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Test case (4.54 KB, patch)
2012-04-02 12:24 PDT, :Ehsan Akhgari
josh: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-07 22:20:03 PST

    
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-07 22:20:09 PST
Created attachment 595318 [details] [diff] [review]
<required>
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-07 22:22:32 PST
Created attachment 595319 [details] [diff] [review]
Add observer notification for last PB docshell going away.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-02-07 22:26:59 PST
Comment on attachment 595319 [details] [diff] [review]
Add observer notification for last PB docshell going away.

Seems ok, but do you need to stabilize the count around going back to session restored stuff, so that we don't end up bouncing off zero for the subframes?
Comment 4 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-07 22:54:24 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/01259925085c

I added an assert to the function to be safe, but I think the counting should be ok.
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-08 04:41:50 PST
Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b8a8bedb49.
Comment 6 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-21 21:29:23 PST
Created attachment 599472 [details] [diff] [review]
Add observer notification for last PB docshell going away.
Comment 7 :Ehsan Akhgari 2012-03-23 15:19:32 PDT
Can this land, Josh?
Comment 8 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-03-24 07:54:17 PDT
Yes, I just haven't got around to it.
Comment 10 :Ehsan Akhgari 2012-04-02 12:24:05 PDT
Created attachment 611548 [details] [diff] [review]
Test case
Comment 11 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-02 12:52:49 PDT
I don't think the whole window close + gc dance should be necessary; this is meant to fire after the last docshell leaves private mode. Why doesn't setting the privateWindow flag to false trigger it?
Comment 12 :Ehsan Akhgari 2012-04-02 13:58:17 PDT
Because the value is decremented when the docshell gets destroyed, and that happens only when a CC is run after the window has been closed.

Do you have any better suggestions?
Comment 13 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-02 17:57:43 PDT
Comment on attachment 611548 [details] [diff] [review]
Test case

It makes me sad that this is necessary, but ok.
Comment 14 Marco Bonardo [::mak] 2012-04-03 02:00:29 PDT
https://hg.mozilla.org/mozilla-central/rev/9065e98ffb5d

leaving open for the test
Comment 16 Marco Bonardo [::mak] 2012-04-04 04:51:07 PDT
https://hg.mozilla.org/mozilla-central/rev/ccb232b6b97a

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