1299998 - "Share this page" button for twitter opens odd tab(s)

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

STR:

1. Press "Share this page" button and choose Twitter to share current active tab.
2. Submit it.
3. Click close button in the panel.

Actual Result:

The close button doesn't work and after clicking outside the panel to close, "data:text/plain;charset=utf8," is opened with new tab.

Expected Result:

The close button should work or there shouldn't be. And closing the panel shouldn't cause opening the odd tab.


Additionally, when you click "Share this page" button after this, share page is also opened in new tab (e.g., <https://twitter.com/intent/tweet?url=https%3A%2F%2Ftwitter.com%2F&text=Twitter>). I cannot reproduce this bug at first time to click the button.

I can reproduce this bug even with safe mode.

Comment 1

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

I have tested this issue on Windows 10 x64 with the latest Nightly (51.0a1-20160904030201) and managed to reproduce it.
After following the STR from the description, when clicking the close button, nothing happens and also if clicking outside the panel to close, "data:text/plain;charset=utf8," is opened with new tab.

I've performed regression and below you can see the results:

Last good revision: 6e17a469a1b137c535adee7e476f86ee9dfcc071
First bad revision: 4e9b9e217312cbff683fd5ac2402eb58c5c370ec
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=6e17a469a1b137c535adee7e476f86ee9dfcc071&tochange=4e9b
9e217312cbff683fd5ac2402eb58c5c370ec

Comment 2

[Shane Caraveo (:mixedpuppy)]

[Tracking Requested - why for this release]: This is a regression on bug 1253604.


Analysis:

The cause is calling docShell.createAboutBlankContentViewer(null); in social-content.js.  Originally this call was in chrome code, and moving it to the content script fixed the issue.  

The original reason for this call (way way back) was an attempt to preemptively avoid memory use.  IMO that is not a big concern now and I should just remove the call.  Mark, what do you think?

Comment 3

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> The cause is calling docShell.createAboutBlankContentViewer(null); in
> social-content.js.

I think we probably want to ensure the frame is cleared somehow. But I don't understand how that call is causing these symptoms?

> Originally this call was in chrome code, and moving it
> to the content script fixed the issue.

What issue is that? It looks like that call wasn't moved as part of bug 1253604.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #3)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> > The cause is calling docShell.createAboutBlankContentViewer(null); in
> > social-content.js.
> 
> I think we probably want to ensure the frame is cleared somehow. But I don't
> understand how that call is causing these symptoms?
> 
> > Originally this call was in chrome code, and moving it
> > to the content script fixed the issue.
> 
> What issue is that? 

This issue described in comment #0 here, which is exactly the same issue as bug 1253604.  Both caused by an exception when calling createAboutBlankContentViewer.  I never fully understood why the weird behavior after that exception.

A correction after re-reading the bugs, the prior bug was caused by a modal dialog appearing during unload and some interaction between that and createAboutBlankContentViewer (which threw an exception).  Preventing beforeunload stopped the dialog, and createAboutBlankContentViewer stopped throwing an exception.  However in this case, a dialog is not being shown.  I'll have to dig a bit further.  

Also, we apparently added createAboutBlankContentViewer use because the sidebar "leaked" without it.  about:blank never loaded in the sidebar because the sidebar was hidden, and the old docshell leaked on shutdown.  Comments about that are also in bug 1257723.

> It looks like that call wasn't moved as part of bug
> 1253604.

per bug 1253604 comment #29 that code was reoganized by bug 1257723

Comment 5

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> > > Originally this call was in chrome code, and moving it
> > > to the content script fixed the issue.
> > 
> > What issue is that? 
> 
> This issue described in comment #0 here, which is exactly the same issue as
> bug 1253604.

I'm still a little lost here - it seems you are saying the issue in comment 0 was fixed by "moving it to the content script" - but it can't have been fixed by that or this bug wouldn't exist?

> Also, we apparently added createAboutBlankContentViewer use because the
> sidebar "leaked" without it.  about:blank never loaded in the sidebar
> because the sidebar was hidden, and the old docshell leaked on shutdown. 
> Comments about that are also in bug 1257723.

Yeah - although IIRC the same basic issue also applied to the panels and not just the sidebar.

> I'll have to dig a bit further.

Cool - let me know when I can help!

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #5)

> I'm still a little lost here - it seems you are saying the issue in comment
> 0 was fixed by "moving it to the content script" - but it can't have been
> fixed by that or this bug wouldn't exist?


Let me start over.

createAboutBlankContentViewer is again throwing an exception causing strange behavior I don't understand.  

It did this before which is in bug 1253604.  The fix in bug 1253604 consisted of preventing onbeforeunload, which some providers used to show a dialog.  If that dialog was shown, createAboutBlankContentViewer threw an exception.  Bug 1253604 also referenced another bug, bug 1257723, where the call to createAboutBlankContentViewer was moved into a content script.  I got confused between the two, remembering bits and parts of both bugs.

The problem went away for a few versions, it does not occur in 48, 49, 50, but does in 51.

SocialAPI removal possibly changed some flow or timing, not sure what, but it's the logical change in the referenced regression from comment #1.   We still disable onbeforeunload and dialogs do not appear as before.  However, createAboutBlankContentViewer is throwing an exception again and causing the same side affects that I saw in bug 1253604.

Comment 7

[Marcia Knous [:marcia]]

Tracking 51+ for this regression, fairly common operation.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Ok, I was able to create the problem regardless of calling createAboutBlankContentViewer.  After spending a bunch of time trying to track down why, I decided to clean up a bunch of this code to help simplify diagnosing the problem.  There were still a bunch of loop things, and stuff for PanelFrame.jsm (loop was only user).  Once cleaned up and a little bit of code moved around, the problem is gone.  

The key fix is the code removed from browser-social.js (moved to social-content.js) and the event listener for DOMWindowClose being moved to the top level of social-content.js.  Once those were done, things work again as expected.  I suspect it is purely a timing issue on when these things occur, and that the error when calling createAboutBlankContentViewer was just a symptom of the timing issue.  

I want to leave the code cleanup/removal in place (for uplift to aurora as well) just because it is cleaner.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1299998 fix closing of share panel,

Review commit: https://reviewboard.mozilla.org/r/80244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80244/

Comment 10

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

Comment on attachment 8793533 [details]
Bug 1299998 fix closing of share panel,

https://reviewboard.mozilla.org/r/80244/#review78926

LGTM!

::: browser/base/content/social-content.js:27
(Diff revision 1)
> -    gDOMContentLoaded = false;
> -    gHookedWindowCloseForPanelClose = false;
> -  }
> -}, true);
>  
> -var gDOMTitleChangedByUs = false;
> +addEventListener("load", function(event) {

Can this go into DOMContentLoaded above? IIRC, it's possible script might have executed by the time "load" fires, which means the script might have already done what this is trying to help?

::: browser/base/content/social-content.js:69
(Diff revision 1)
> +}
> +
> +function disableDialogs() {
> +  let windowUtils = content.QueryInterface(Ci.nsIInterfaceRequestor).
> +                    getInterface(Ci.nsIDOMWindowUtils);
> +  windowUtils.disableDialogs();  

nit: trailing whitespace

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8793533 [details]
Bug 1299998 fix closing of share panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80244/diff/1-2/

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8793533 [details]
Bug 1299998 fix closing of share panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80244/diff/2-3/

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8793533 [details]
Bug 1299998 fix closing of share panel,

https://reviewboard.mozilla.org/r/80244/#review78926

> Can this go into DOMContentLoaded above? IIRC, it's possible script might have executed by the time "load" fires, which means the script might have already done what this is trying to help?

yep, moved it.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8793533 [details]
Bug 1299998 fix closing of share panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80244/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb75100b883d
fix closing of share panel, r=markh

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8793533 [details]
Bug 1299998 fix closing of share panel,

Approval Request Comment
[Feature/regressing bug #]:share
[User impact if declined]:share becomes unusable after cancelling a share
[Describe test coverage new/current, TreeHerder]:manual testing
[Risks and why]: low.  the patch looks larger, but there is a lot of removal of code that was no longer used after hello was removed.  Only the share panel would be affected by the changes.
[String/UUID change made/needed]:none

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb75100b883d

Comment 18

[Gerry Chang [:gchang]]

Hi Florin, 
Can we get someone's help to verify this in nightly first?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I confirmed that this is completely fixed in today's Nightly. Thank you!

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8793533 [details]
Bug 1299998 fix closing of share panel,

Fix a regression related to "Share this page". Take it in 51 aurora.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a19443bce1d

Comment 22

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 51.0a1, build ID: 20160901030202, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 51.0a2, build ID: 20161113004006, on Windows 10 x64.

Cheers!