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]
:
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]
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]
no flags Details | Diff | Review
Add observer notification for last PB docshell going away. (3.01 KB, patch)
2012-02-07 22:22 PST, Josh Matthews [:jdm]
bzbarsky: review+
Details | Diff | Review
Add observer notification for last PB docshell going away. (3.19 KB, patch)
2012-02-21 21:29 PST, Josh Matthews [:jdm]
no flags Details | Diff | Review
Test case (4.54 KB, patch)
2012-04-02 12:24 PDT, :Ehsan Akhgari (busy, don't ask for review please)
josh: review+
Details | Diff | Review

Description Josh Matthews [:jdm] 2012-02-07 22:20:03 PST

    
Comment 1 Josh Matthews [:jdm] 2012-02-07 22:20:09 PST
Created attachment 595318 [details] [diff] [review]
<required>
Comment 2 Josh Matthews [:jdm] 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] 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] 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] 2012-02-08 04:41:50 PST
Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b8a8bedb49.
Comment 6 Josh Matthews [:jdm] 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 (busy, don't ask for review please) 2012-03-23 15:19:32 PDT
Can this land, Josh?
Comment 8 Josh Matthews [:jdm] 2012-03-24 07:54:17 PDT
Yes, I just haven't got around to it.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-02 10:59:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9065e98ffb5d
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-02 12:24:05 PDT
Created attachment 611548 [details] [diff] [review]
Test case
Comment 11 Josh Matthews [:jdm] 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 (busy, don't ask for review please) 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] 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 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-03 12:18:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb232b6b97a
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.