The default bug view has changed. See this FAQ.

sidebar docShell is not defined after sidebar has been unloaded

VERIFIED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mccr8, Assigned: Felipe)

Tracking

Trunk
Firefox 19
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified, firefox19 verified)

Details

(Whiteboard: [Fx17])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → felipc
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
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
Blocks: 802435
Summary: sidebar remains blank after watching fullscreen YouTube video → sidebar docShell is not defined after sidebar has been unloaded
(Assignee)

Comment 2

5 years ago
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.
Attachment #675812 - Flags: review?(gavin.sharp)
(Assignee)

Comment 3

5 years ago
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() {
Attachment #675812 - Attachment is patch: true
(Assignee)

Comment 4

5 years ago
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.
Attachment #675814 - Flags: review?(gavin.sharp)
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.
Duplicate of this bug: 806292
(Assignee)

Updated

5 years ago
Whiteboard: [Fx17]
tracking-firefox17: --- → +
tracking-firefox18: --- → +
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.
Attachment #675812 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/215b45694fff
(Assignee)

Comment 9

5 years ago
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
Attachment #676464 - Flags: review+
Attachment #676464 - Flags: approval-mozilla-beta?
Attachment #676464 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

5 years ago
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
Attachment #675814 - Flags: review?(gavin.sharp)
Attachment #676464 - Flags: approval-mozilla-beta?
Attachment #676464 - Flags: approval-mozilla-beta+
Attachment #676464 - Flags: approval-mozilla-aurora?
Attachment #676464 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/cf58ad84322c
status-firefox17: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/215b45694fff
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
https://hg.mozilla.org/releases/mozilla-aurora/rev/0be4b56a0c58
status-firefox18: --- → fixed
status-firefox19: --- → fixed
(Assignee)

Comment 14

4 years ago
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
Attachment #675814 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
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?
Attachment #681864 - Flags: review?(gavin.sharp)
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.
Attachment #681864 - Flags: review?(gavin.sharp) → feedback+
Andrew, are you able to reproduce this bug anymore? It should be fixed in 17, 18, and 19.
(Reporter)

Comment 18

4 years ago
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.
Thanks Andrew, confirmed this is also fixed in Firefox 17, 18, and 19.
Status: RESOLVED → VERIFIED
status-firefox17: fixed → verified
status-firefox18: fixed → verified
status-firefox19: fixed → verified
You need to log in before you can comment on or make changes to this bug.