Share functionality ceases to work correctly after canceling a Facebook post using the "Share This Page" option

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: cmuresan, Assigned: mixedpuppy)

Tracking

47 Branch
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed, firefox49 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Posted video ShareBug.mp4
[Affected versions]:
- Firefox 44.0.2
- Firefox 45beta10
- Latest Aurora 46.0a2, Build ID 20160304004008
- Latest Nightly 47.0a1, Build ID 20160303030253

[Affected platforms]:
- Windows 7 x32/64, 8.1 x32, 10 x64
- Mac OS X 10.11
- Ubuntu 14.04

[Prerequisites]:
- Add the Share This Page feature to the Menu (if not already added) from customization menu.

[Steps to reproduce]:
1. Open the browser and navigate to a site.
2. Right click on the page and choose "Share This Page" option.
3. Add the Facebook service and log in.
4. In the Facebook Post form click "Cancel" button.
5. When the "Are you sure?" pop-up is displayed, Click "Leave Page" button.
6. Right click again on the page and select "Share This Page" option.
7. Observe the browser behavior.

[Expected result]:
- The container is opened and populated with a Facebook Post form containing the page in question.

[Actual result]:
- A new tab is opened at the same time the container is displayed. The tab contains all the information that should be in the container. 
- Clicking anywhere on the page opens an empty tab with "data:text/plain;charset=utf8," autocompleted in the URL bar.
- After this the Share functionality no longer works with any service.

[Regression range]:
- This doesn't seem to be a regression as the issue is reproducible on Nightly builds from 2014-10-10. Builds before this date do not have the Share feature.

[Additional notes]:
- The Share functionality works correctly only after Firefox has been restarted.
Assignee

Comment 1

3 years ago
I don't understand WHY this is happening, but this does fix the problem.  I'm taking a guess that the "stay on page" dialog is interfering somehow.  In any case, I do believe that the removed call is not necessary to unload the page, we were being overly aggressive with that.
Attachment #8726958 - Flags: review?(markh)
Assignee

Updated

3 years ago
Assignee: nobody → mixedpuppy
Comment on attachment 8726958 [details] [diff] [review]
fix share after cancelling on facebook

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

This change worries me - bug 944219 added a similar call to avoid leaks, and I seem to remember that the unload didn't automatically end up happening due to the item being hidden. I think we should try and better understand what is going on before we potentially allow the frame to not die.
Assignee

Comment 3

3 years ago
Here's a more appropriate fix, prevents the beforeunload dialogs which are bad ux in this case anyway.  The dialog was appearing while the document was also being destroyed in the panel onhidden.
Attachment #8726958 - Attachment is obsolete: true
Attachment #8726958 - Flags: review?(markh)
Attachment #8730287 - Flags: review?(markh)
Comment on attachment 8730287 [details] [diff] [review]
fix share after cancelling on facebook

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

This looks like a much better approach, any chance of tests? I'm also wondering if the same problem is going to exist for other bits of the social UI? Facebook is showing this for the share panel, but I don't see why the sidebar etc wouldn't exhibit the same behaviour if a provider tried - tests might also help determine if that's possible or not. If that does turn out to be true, it seems like the message could be more generic - something like Social:InitializeSocialFrame or something...
Assignee

Comment 5

3 years ago
Tests already exist for checking beforeunload with dialogs disabled in layout.  

I'd actually be fine doing this globally for all frames that load social-content.js, it could just be done in DOMContentLoaded (already have a listener for that).  However that would also affect chatwindows for hello...Standard8, do you have any issue with that?
Flags: needinfo?(standard8)
I can't remember if I already tried to answer this or not.

AFAIK we don't have an issue with blocking beforeUnload - we tend to just modify what the close/hangup buttons do.
Flags: needinfo?(standard8)
Assignee

Comment 8

3 years ago
updates share tests to try onbeforeunload, also reenabling share tests (will see if window leaks)
Attachment #8730287 - Attachment is obsolete: true
Attachment #8730287 - Flags: review?(markh)
Attachment #8739594 - Flags: review?(markh)
Comment on attachment 8739594 [details] [diff] [review]
fix share after cancelling on facebook

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

LGTM, but if possible I think we should be adding a new test file instead of changing this existing one (well, maybe the existing one needs a change, but listening for this event seems like it should remain. I guess you can ignore this if there are other existing tests for this event...)

::: browser/base/content/test/social/share.html
@@ -1,5 @@
>  <html>
>    <head>
>      <meta charset="utf-8">
>      <script>
> -      addEventListener("OpenGraphData", function(e) {

should we keep the event listener and add the new functionality as a new test?
Attachment #8739594 - Flags: review?(markh) → review+
Assignee

Updated

3 years ago
Blocks: 1115131
Assignee

Comment 11

3 years ago
This separates out the new test from the previously existing tests, fixes some async issues that likely were the cause of bug 1115131.
Attachment #8739594 - Attachment is obsolete: true
Attachment #8744418 - Flags: review?(markh)
Comment on attachment 8744418 [details] [diff] [review]
fix share after cancelling on facebook

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

LGTM, thanks.

::: browser/base/content/test/social/browser_share.js
@@ +367,5 @@
> +          EventUtils.sendKey("f");
> +          EventUtils.sendKey("a");
> +          EventUtils.sendKey("i");
> +          EventUtils.sendKey("l");
> +  

trailing whitespace
Attachment #8744418 - Flags: review?(markh) → review+
Backed out in https://hg.mozilla.org/integration/fx-team/rev/f2b20c6eb71dbf7a2884a776251e760e73b8c69b

There may be more things in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=5fdb0bf1527a446f946d3585371f4da0ab33c447 which were from this rather than from that push, I don't know, but at least the browser_share.js parts were from this, since https://treeherder.mozilla.org/logviewer.html#?job_id=8984029&repo=fx-team was on the push for this.
Assignee

Comment 16

3 years ago
change disables css animation during panel tests, fixes intermittent bug 1115131 that appeared during attempt to land.  carry forward r+
Attachment #8744418 - Attachment is obsolete: true
Attachment #8746067 - Flags: review+

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/93c744e5c550
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee

Comment 19

3 years ago
Comment on attachment 8746067 [details] [diff] [review]
fix share after cancelling on facebook

Approval Request Comment
[Feature/regressing bug #]:share
[User impact if declined]:hitting cancel in facebook share panel breaks future attempts to share
[Describe test coverage new/current, TreeHerder]:share tests had been disabled, patch fixes and reenables tests, adds test for this particular situation.
[Risks and why]: low
[String/UUID change made/needed]:none
Attachment #8746067 - Flags: approval-mozilla-beta?
Attachment #8746067 - Flags: approval-mozilla-aurora?
Hello Ciprian, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(ciprian.muresan)
Comment on attachment 8746067 [details] [diff] [review]
fix share after cancelling on facebook

Fixing fb share scenario (after it was canceled once), patch includes new automated test which is great, Aurora48+, Beta47+
Attachment #8746067 - Flags: approval-mozilla-beta?
Attachment #8746067 - Flags: approval-mozilla-beta+
Attachment #8746067 - Flags: approval-mozilla-aurora?
Attachment #8746067 - Flags: approval-mozilla-aurora+
Hi,

I have retested the issue on the latest Nightly(49.0a1, Build ID 20160505030327) and I can reproduce the issue on my (possibly) dirty profile, by logging out of Facebook and logging back in step 3 of the original STR. 
I can also reproduce the issue with a new profile using the original STR.
Tested on Windows 10 x64, Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.11 and the issue is reproducible on all platforms.

However, after encountering the issue the first time on the new profile and restarting Firefox, the issue is no longer reproducible. But if I log out of Facebook and then log back in, the issue is again reproducible.

I've followed the pushlog for the latest Nightly and the fix is surely in today's build. Any idea what might be causing this?
Flags: needinfo?(rkothari)
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(ciprian.muresan)
I uplifted this to aurora despite comment 22. The patch had errors when I tried to apply it to beta, so didn't uplift it there.
Assignee

Comment 25

3 years ago
Verified there is still a bug on this.  A new page load (e.g. login via share panel then switches to share page) needs to re-disable the dialogs.
Flags: needinfo?(mixedpuppy)
Assignee

Comment 26

3 years ago
A patch I've been working on for bug 1257723 actually fixes the new str, I'll try to pull out the share relevant fixes for this.
Comment on attachment 8746067 [details] [diff] [review]
fix share after cancelling on facebook

Reverting the Beta uplift flag as this fix is not complete. Please renom updated patch for uplift to Beta47.
Flags: needinfo?(rkothari)
Attachment #8746067 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
(In reply to Ciprian Muresan [:cmuresan] from comment #22)
> Hi,
> 
> I have retested the issue on the latest Nightly(49.0a1, Build ID
> 20160505030327) and I can reproduce the issue on my (possibly) dirty
> profile, by logging out of Facebook and logging back in step 3 of the
> original STR. 
> I can also reproduce the issue with a new profile using the original STR.
> Tested on Windows 10 x64, Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.11
> and the issue is reproducible on all platforms.
> 
> However, after encountering the issue the first time on the new profile and
> restarting Firefox, the issue is no longer reproducible. But if I log out of
> Facebook and then log back in, the issue is again reproducible.
> 
> I've followed the pushlog for the latest Nightly and the fix is surely in
> today's build. Any idea what might be causing this?

Thank you for a prompt verification! It was extremely valuable. Our dev will be updating the patch with additional fix needed to handle the re-login scenario.
Assignee

Comment 29

3 years ago
Making this depend on changes in bug 1257723 which addresses the issues I described in comment 25.  If this bug uplifts, we'll need to uplift bug 1257723 also.  For that reason, I'm going to skip on a fix for beta and only request uplift to aurora.
Depends on: 1257723
Assignee

Updated

3 years ago
Duplicate of this bug: 1208801
Assignee

Updated

3 years ago
Depends on: 1299998

Updated

4 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.