Closed Bug 904104 Opened 6 years ago Closed 6 years ago

share panel goes blank

Categories

(Firefox Graveyard :: SocialAPI, defect)

23 Branch
x86
macOS
defect
Not set

Tracking

(firefox23 wontfix, firefox24 wontfix, firefox25+ verified, firefox26 verified)

VERIFIED FIXED
Firefox 27
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 + verified
firefox26 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Details

Attachments

(2 files, 4 obsolete files)

I noticed this issue during a demo last week.  We did a share (I think via facebook, don't remember) then the share panel went blank.  After that, we could no longer do any share on any tab, had to restart firefox.  I believe this was on nightly.  Need to see if we can reproduce the issue.
Finally figured this out.

STR:

- install 2 providers, at least on that supports share
- with first provider selected in sidebar
- share a page with the second provider

If that share page calls window.close() after the share, the share panel becomes blank and will no longer function until restart.

Solution for 23/24:
- providers should not call window.close in the share panel.  show a landing page.  next time user opens the panel it will refresh.

Solution for 25:
- port part of the multiprovider patch from 26 that modifies MozSocialAPI.jsm, removing the requirement for workerURL to exist (thus allowing the DOMWindowClose handler to be used)

Fx26 is unaffected since it already has the fix in MozSocialAPI.jsm.
Attached patch fix-close.patch (obsolete) — Splinter Review
proposed patch, untested.
Attachment #804118 - Flags: feedback?(mhammond)
Comment on attachment 804118 [details] [diff] [review]
fix-close.patch

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

I think we should take the minimum required - eg, drop the change for localName, var->let etc just to minimize risk, but yeah, this looks reasonable.
Attachment #804118 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 804118 [details] [diff] [review]
fix-close.patch

Actually, this fix would depend on running multiple workers.  I have to come up with an alternate patch that will place the event listener on each frame.

Fx26 is affected unless social.allowMultipleWorkers = true.
Attachment #804118 - Attachment is obsolete: true
Attached patch 904104-close (obsolete) — Splinter Review
This fixes handling window.close for any social panel, regardless of multiworker support.  I have an aurora patch in the wings.

I've also pushed a fix to the demo provider share panel to make it call close if mozsocial does not exist (it tries to send the share data to the sidebar if it is open).  So you can test close with the demo provider, just have a different provider selected in the sidebar (e.g. fb)
Assignee: nobody → mixedpuppy
Attachment #804146 - Flags: feedback?(mhammond)
> I think we should take the minimum required - eg, drop the change for
> localName, var->let etc just to minimize risk, but yeah, this looks
> reasonable.

using localName is required to properly support close from a torn off chat window, for some reason in that window, nodeName appears as "xul:chatbox".
Comment on attachment 804146 [details] [diff] [review]
904104-close

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

Chatting on IRC, I'm not sure this is ultimately correct (eg, "share only" providers might ultimately want access to more of mozSocial than just fancy close behaviour), but I think it's an improvement and shouldn't cause other issues.

But please move the close handling into a utility function and have injectController call it.
Attachment #804146 - Flags: feedback?(mhammond) → feedback+
(In reply to Mark Hammond (:markh) from comment #7)

> Chatting on IRC, I'm not sure this is ultimately correct (eg, "share only"
> providers might ultimately want access to more of mozSocial than just fancy
> close behaviour)

That will happen automatically once multiple workers is turned on.
Attached patch 904104-close fx-team (obsolete) — Splinter Review
feedback taken, patch minimized.
Attachment #804146 - Attachment is obsolete: true
Attachment #804616 - Flags: review?(felipc)
Attached patch 904104-close aurora branch (obsolete) — Splinter Review
Comment on attachment 804616 [details] [diff] [review]
904104-close fx-team

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

Can you add a comment saying why handleWindowClosed is called even for non-enabled providers?

Also, does it make sense to have the call inside the getProvider -> if (provider), even if it's not enabled?
Attachment #804616 - Flags: review?(felipc) → review+
feedback and try

https://tbpl.mozilla.org/?tree=Try&rev=17de2abb4047
Attachment #804616 - Attachment is obsolete: true
Attachment #804742 - Flags: review+
(In reply to :Felipe Gomes from comment #11)
> Comment on attachment 804616 [details] [diff] [review]

> Also, does it make sense to have the call inside the getProvider -> if
> (provider), even if it's not enabled?

If for some reason we hit some extreme edge case: a social panel is opened and the getProvider call fails, and the content in the social panel calls window.close, then the panel is unusable until restart.  

I cant think of any reason to avoid adding this handler on social panels.
Comment on attachment 804742 [details] [diff] [review]
904104-close fx-team

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 891216 and share
User impact if declined: if content loaded into a social panel calls window.close the panel becomes unusable again until firefox restarts
Testing completed (on m-c, etc.): fx-team
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #804742 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/926d081e02d6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
I'm not sure how common the STR in comment 1 are, and given the fact that this appears to have affected earlier versions, I'm leaning toward not tracking but uplifting to Beta 25.
(In reply to Alex Keybl [:akeybl] from comment #18)
> I'm not sure how common the STR in comment 1 are, and given the fact that
> this appears to have affected earlier versions, I'm leaning toward not
> tracking but uplifting to Beta 25.

If we can uplift the patch to beta, I think that is fine.  The STR will be more common as share providers come online.
Since we are looking to have more providers in upcoming versions it can't hurt to track this and make sure this gets an early uplift to Beta 25.
Comment on attachment 804742 [details] [diff] [review]
904104-close fx-team

[Triage Comment]
Approving for early Beta uplift so we get bake & test time.
Attachment #804742 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e739e49b953c in the Great Bug 916757 Purge.

Feel free to reland after a debug Windows try push with like 30 browser-chrome runs on WinXP and Win8, since we're still not especially sure what got rid of it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 26 → ---
Comment on attachment 804742 [details] [diff] [review]
904104-close fx-team

relanded: https://hg.mozilla.org/integration/fx-team/rev/596116e9c42c

This was backed out along with several others due to winXP crashes, I narrowed the crash down to bug 914435

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 916757
User impact if declined: some social panels can become unusable until restarting firefox if the content in that panel calls window.close
Testing completed (on m-c, etc.): fx-team
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Also needs uplift to beta per previous approval.
Attachment #804742 - Flags: approval-mozilla-aurora?
Comment on attachment 804742 [details] [diff] [review]
904104-close fx-team

I was premature in approving for Beta - will come back around to check on this tomorrow once it's been merged to central without issues.
Attachment #804742 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Attachment #804745 - Attachment description: 904104-close aurora branch → 904104-close beta branch
https://hg.mozilla.org/mozilla-central/rev/596116e9c42c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Attachment #804742 - Flags: approval-mozilla-beta?
Attachment #804742 - Flags: approval-mozilla-beta+
Attachment #804742 - Flags: approval-mozilla-aurora?
Attachment #804742 - Flags: approval-mozilla-aurora+
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20130923194050

Verified as fixed on Firefox 25 beta 2 - the share panel is not blank when sharing and having multiple providers on (I used the Demo Social Service 2 and the Facebook messenger).
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora 26.0a2 (buildID: 20131003004003) using Facebook Messenger and Cliqz.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.