Last Comment Bug 806038 - sidebar docShell is not defined after sidebar has been unloaded
: sidebar docShell is not defined after sidebar has been unloaded
Status: VERIFIED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Firefox 19
Assigned To: :Felipe Gomes (needinfo me!)
:
Mentors:
: 806292 (view as bug list)
Depends on:
Blocks: 802435
  Show dependency treegraph
 
Reported: 2012-10-26 20:02 PDT by Andrew McCreight [:mccr8]
Modified: 2012-12-04 15:14 PST (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
verified


Attachments
Patch (5.60 KB, patch)
2012-10-26 23:25 PDT, :Felipe Gomes (needinfo me!)
gavin.sharp: review+
Details | Diff | Review
Patch with refactoring (13.76 KB, patch)
2012-10-26 23:35 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
Patch as landed (6.01 KB, patch)
2012-10-29 21:00 PDT, :Felipe Gomes (needinfo me!)
felipc: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Review
Updated patch with refactoring (10.30 KB, patch)
2012-11-14 23:01 PST, :Felipe Gomes (needinfo me!)
gavin.sharp: feedback+
Details | Diff | Review

Description Andrew McCreight [:mccr8] 2012-10-26 20:02:52 PDT
I have the FB sidebar open, on a page with a YouTube video.  I start playing the video, switch it to full screen, watch it for a little bit, then go back to viewing the video in non-full screen, and the sidebar is white.  Clicking on it doesn't fix it, and it remains blanked out for multiple minutes afterwards.  Opening and closing the sidebar a few times is required to get it to reload.
Comment 1 :Felipe Gomes (needinfo me!) 2012-10-26 23:10:51 PDT
If you turn the feature off and on again, or if you let the sidebar unload after the timeout from bug 802435, we can't get to the docshell of the sidebar. The docshell is already created, but browser.xml uses this.boxObject.QI(nsIContainerObject).docShell, and since the sidebar is hidden, boxObject doesn't work.

The solution here is to specifically listen for the docshell re-creation when we remove the sidebar, and then reattach the progress listener and set the proper docShell fields. This should get rid of the problem once for all
Comment 2 :Felipe Gomes (needinfo me!) 2012-10-26 23:25:53 PDT
Created attachment 675812 [details] [diff] [review]
Patch

This patch basically reverts the addProgressListener changes made in bug 802435 and adds new code on unloadSidebar to wait for the docShell re-creation and add the listeners again.

It also fixes another bug (unrelated to 802435) where the load callback was actually never being called. Changing it to a bubbling listener fixed it. This might be the cause of some of the cases where it was still possible to see the sidebar on both facebook's page and the sidebar.
Comment 3 :Felipe Gomes (needinfo me!) 2012-10-26 23:26:06 PDT
Comment on attachment 675812 [details] [diff] [review]
Patch

># HG changeset patch
># Parent 58c8080a1a7c35e71fa839959bfe12baadb9baa0
># User Felipe Gomes <felipc@gmail.com>
>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
>--- a/browser/base/content/browser-social.js
>+++ b/browser/base/content/browser-social.js
>@@ -891,16 +891,23 @@ var SocialToolbar = {
>                                              encodeURIComponent(src), null, null, null, null);
>     sizeSocialPanelToContent(aNotificationFrame);
>   }
> }
> 
> var SocialSidebar = {
>   // Called once, after window load, when the Social.provider object is initialized
>   init: function SocialSidebar_init() {
>+    let sbrowser = document.getElementById("social-sidebar-browser");
>+    // setting isAppTab causes clicks on untargeted links to open new tabs
>+    sbrowser.docShell.isAppTab = true;
>+    this.errorListener = new SocialErrorListener("sidebar");
>+    sbrowser.webProgress.addProgressListener(this.errorListener,
>+                                             Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
>+                                             Ci.nsIWebProgress.NOTIFY_LOCATION);
>     this.updateSidebar();
>   },
> 
>   // Whether the sidebar can be shown for this window.
>   get canShow() {
>     return Social.uiVisible && Social.provider.sidebarURL && !this.chromeless;
>   },
> 
>@@ -917,33 +924,40 @@ var SocialSidebar = {
>     return Services.prefs.getBoolPref("social.sidebar.open") && !document.mozFullScreen;
>   },
> 
>   dispatchEvent: function(aType, aDetail) {
>     let sbrowser = document.getElementById("social-sidebar-browser");
>     let evt = sbrowser.contentDocument.createEvent("CustomEvent");
>     evt.initCustomEvent(aType, true, true, aDetail ? aDetail : {});
>     sbrowser.contentDocument.documentElement.dispatchEvent(evt);
>+    switch (aType) {
>+      case "socialFrameShow":
>+        sbrowser.docShell.isActive = true;
>+        break;
>+      case "socialFrameHide":
>+        sbrowser.docShell.isActive = false;
>+        break;
>+    }
>   },
> 
>   updateSidebar: function SocialSidebar_updateSidebar() {
>     clearTimeout(this._unloadTimeoutId);
>     // Hide the toggle menu item if the sidebar cannot appear
>     let command = document.getElementById("Social:ToggleSidebar");
>     command.setAttribute("hidden", this.canShow ? "false" : "true");
> 
>     // Hide the sidebar if it cannot appear, or has been toggled off.
>     // Also set the command "checked" state accordingly.
>     let hideSidebar = !this.canShow || !this.opened;
>     let broadcaster = document.getElementById("socialSidebarBroadcaster");
>     broadcaster.hidden = hideSidebar;
>     command.setAttribute("checked", !hideSidebar);
> 
>     let sbrowser = document.getElementById("social-sidebar-browser");
>-    sbrowser.docShell.isActive = !hideSidebar;
>     if (hideSidebar) {
>       this.dispatchEvent("socialFrameHide");
>       // If we've been disabled, unload the sidebar content immediately;
>       // if the sidebar was just toggled to invisible, wait a timeout
>       // before unloading.
>       if (!this.canShow) {
>         this.unloadSidebar();
>       } else {
>@@ -951,29 +965,24 @@ var SocialSidebar = {
>           this.unloadSidebar,
>           Services.prefs.getIntPref("social.sidebar.unload_timeout_ms")
>         );
>       }
>     } else {
>       // Make sure the right sidebar URL is loaded
>       if (sbrowser.getAttribute("origin") != Social.provider.origin) {
>         sbrowser.setAttribute("origin", Social.provider.origin);
>-        // setting isAppTab causes clicks on untargeted links to open new tabs
>-        sbrowser.docShell.isAppTab = true;
>-        sbrowser.webProgress.addProgressListener(new SocialErrorListener("sidebar"),
>-                                                 Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
>-                                                 Ci.nsIWebProgress.NOTIFY_LOCATION);
>         sbrowser.setAttribute("src", Social.provider.sidebarURL);
>         sbrowser.addEventListener("load", function sidebarOnShow() {
>-          sbrowser.removeEventListener("load", sidebarOnShow);
>+          sbrowser.removeEventListener("load", sidebarOnShow, true);
>           // let load finish, then fire our event
>           setTimeout(function () {
>             SocialSidebar.dispatchEvent("socialFrameShow");
>           }, 0);
>-        });
>+        }, true);
>       } else {
>         this.dispatchEvent("socialFrameShow");
>       }
>     }
>   },
> 
>   unloadSidebar: function SocialSidebar_unloadSidebar() {
>     let sbrowser = document.getElementById("social-sidebar-browser");
>@@ -982,16 +991,30 @@ var SocialSidebar = {
> 
>     // Bug 803255 - If we don't remove the sidebar browser from the DOM,
>     // the previous document leaks because it's only released when the
>     // sidebar is made visible again.
>     let container = sbrowser.parentNode;
>     container.removeChild(sbrowser);
>     sbrowser.removeAttribute("origin");
>     sbrowser.removeAttribute("src");
>+
>+    function resetDocShell(docshellSupports) {
>+      let docshell = docshellSupports.QueryInterface(Ci.nsIDocShell);
>+      if (docshell.chromeEventHandler != sbrowser)
>+        return;
>+      docshell.isAppTab = true;
>+      docshell.QueryInterface(Ci.nsIWebProgress)
>+              .addProgressListener(SocialSidebar.errorListener,
>+                                   Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
>+                                   Ci.nsIWebProgress.NOTIFY_LOCATION);
>+      Services.obs.removeObserver(resetDocShell, "webnavigation-create", false);
>+    }
>+    Services.obs.addObserver(resetDocShell, "webnavigation-create", false);
>+
>     container.appendChild(sbrowser);
> 
>     SocialFlyout.unload();
>   },
> 
>   _unloadTimeoutId: 0,
> 
>   setSidebarErrorMessage: function() {
Comment 4 :Felipe Gomes (needinfo me!) 2012-10-26 23:35:52 PDT
Created attachment 675814 [details] [diff] [review]
Patch with refactoring

(ignore the previous comment, I clicked Edit Attachment As Comment and didn't realize it)


Now this is a slightly bigger patch, but it is basically the same thing as the previous patch. The extra is that it refactors the docshell creation function to reuse it for the content panels and the flyout as well. It has the benefit of making the handling of all of them simpler, and also allows us to remove that hack to force a layout flush for the flyout.

I will let you make the call on which patch we should use now. I think this one definitely is better in the long run, but it is certainly bigger for beta.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-10-27 12:29:37 PDT
This is the try run that Felipe sent of the bigger patch,
https://tbpl.mozilla.org/?tree=Try&rev=2d324bfb018d

All tests were green, the only oranges on the push were previously-known intermittent oranges.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-29 10:57:12 PDT
*** Bug 806292 has been marked as a duplicate of this bug. ***
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-29 18:02:30 PDT
Comment on attachment 675812 [details] [diff] [review]
Patch

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

> var SocialSidebar = {

>   init: function SocialSidebar_init() {

>+    sbrowser.docShell.isAppTab = true;
>+    sbrowser.webProgress.addProgressListener(this.errorListener,
>+                                             Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
>+                                             Ci.nsIWebProgress.NOTIFY_LOCATION);

Can you factor this bit into a helper, and call it from resetDocShell as well?

>   dispatchEvent: function(aType, aDetail) {

Let's rename this to "setSidebarVisibilityState(aEnabled), or somesuch. Also, it should set isActive before calling dispatchEvent.
Comment 8 :Felipe Gomes (needinfo me!) 2012-10-29 20:57:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/215b45694fff
Comment 9 :Felipe Gomes (needinfo me!) 2012-10-29 21:00:00 PDT
Created attachment 676464 [details] [diff] [review]
Patch as landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 802435
User impact if declined: sidebar is blank after unloaded
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): patch mostly revert some changes from 802435
String or UUID changes made by this patch: none
Comment 10 :Felipe Gomes (needinfo me!) 2012-10-29 21:01:03 PDT
Comment on attachment 675814 [details] [diff] [review]
Patch with refactoring

I'll update this refactoring patch soon to have this code clean-up on m-c and perhaps aurora
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-29 23:39:05 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/cf58ad84322c
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-10-30 08:24:12 PDT
https://hg.mozilla.org/mozilla-central/rev/215b45694fff
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-10-30 11:56:25 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/0be4b56a0c58
Comment 14 :Felipe Gomes (needinfo me!) 2012-11-14 23:01:07 PST
Created attachment 681864 [details] [diff] [review]
Updated patch with refactoring

updated to m-c the the version of the patch with refactoring, and fixed the previous review comments (from irc), which was to change the usage of ids to element refs.

Not requesting review for new because bug 811089 and bug 811247 might change the final appearance of this patch. Also I'll do the final pass in a separate bug to not track different landings in this bug, but posting here for now as I want to have it posted somewhere
Comment 15 :Felipe Gomes (needinfo me!) 2012-11-19 18:02:40 PST
Comment on attachment 681864 [details] [diff] [review]
Updated patch with refactoring

Not the entire patch will be necessary after bug 811089, but 811089 won't be uplifted and I'd like to get this refactoring into aurora/beta if possible. Gavin, can you take a look and see what you think?
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-21 13:13:09 PST
Comment on attachment 681864 [details] [diff] [review]
Updated patch with refactoring

This looks like a good cleanup, I think we should get a separate bug filed for it. Less certain about uplifting it, but we can discuss further in the new bug.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 14:45:44 PST
Andrew, are you able to reproduce this bug anymore? It should be fixed in 17, 18, and 19.
Comment 18 Andrew McCreight [:mccr8] 2012-12-04 15:10:33 PST
Works for me on 20. The sidebar is initially white after I come back from the full screen video (indicating it has been unloaded, I guess), but it immediately reloads.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 15:14:20 PST
Thanks Andrew, confirmed this is also fixed in Firefox 17, 18, and 19.

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