813789 - console error: TypeError: SocialFlyout._dynamicResizer is null

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

I'm having trouble reproducing this, but certainly saw it once.  The error is in SocialFlyout.onshown(), inside a setTimeout(0) which is inside a load listener.  It seems that if the flyout is hidden while the flyout document is still loading, SocialFlyout.onHidden() will set _dynamicResizer to null before the load event (or the setTimeout) fires.

FWIW, I was working on a bug related to social.active/social.enabled, so was in the process of toggling these via the UI - so to reproduce it might actually be necessary to toggle enabled/active before the load event fires rather than simply causing the panel to hide "normally".

This shouldn't actually cause a real problem other than the console message.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: check _dynamicResizer

a trivial fix which just checks the resizer isn't null.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 683866 [details] [diff] [review]
check _dynamicResizer

Review of attachment 683866 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +386,5 @@
>          iframe.removeEventListener("load", panelBrowserOnload, true);
>          setTimeout(function() {
> +          if (SocialFlyout._dynamicResizer) { // may go null if hidden quickly
> +            SocialFlyout._dynamicResizer.start(panel, iframe);
> +            SocialFlyout.dispatchPanelEvent("socialFrameShow");

Does the |SocialFlyout.dispatchPanelEvent("socialFrameShow");| need to be within the if-block? Seems like we would always want to dispatch the event.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jared Wein [:jaws] from comment #2)
> Comment on attachment 683866 [details] [diff] [review]
> check _dynamicResizer
> 
> Review of attachment 683866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-social.js
> @@ +386,5 @@
> >          iframe.removeEventListener("load", panelBrowserOnload, true);
> >          setTimeout(function() {
> > +          if (SocialFlyout._dynamicResizer) { // may go null if hidden quickly
> > +            SocialFlyout._dynamicResizer.start(panel, iframe);
> > +            SocialFlyout.dispatchPanelEvent("socialFrameShow");
> 
> Does the |SocialFlyout.dispatchPanelEvent("socialFrameShow");| need to be
> within the if-block? Seems like we would always want to dispatch the event.

The issue is that the panel was hidden before the onload event fired.  Thus, if we always sent the event, it could be sent when the panel is hidden, which is almost certainly the wrong things to do.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

OK that makes sense. Maybe include a comment to describe the relationship here?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Oops - sorry, I landed before seeing your request to add a comment.  OTOH though, the comment I did add says "// may go null if hidden quickly".  Would you like me to file a followup?

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c13bcee3d6e

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

It's fine, no big deal.

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2c13bcee3d6e