1279285 - [New Tab Page] thumbnail service opens a popup

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[[:philipp]]

hi, we had the following problem reported at sumo which is reproducible: https://support.mozilla.org/questions/1126374

str in a new profile:
- open https://collaborate.avaya.com/aacpa/ and generally allow the domain to open popups
- close this tab again and open the new tab page
- shortly after about:newtab opens, the popup window from that domain is opening on its own

running the mozregression tool on this issue gives me this pushlog https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=512c7e8f0030155cee43622aac309761f5af0e66&tochange=a6b93cceaf4e15a45d37cd5e75660cdc7d671b10 and bug 1185343 as likely source of the regression.

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Dolske, based on the fact that this is a regression since 42, I do not think this would be considered a critical issue for Fx47. Please let me know if I am mistaken about the severity of this issue.

Comment 2

[Justin Dolske [:Dolske]]

Drew, is this something you might be able to look at? Seems pretty problematic have the thumbnailer be opening popups. (Not a disaster if it's only pages that the user opted-in to popups, but still gross.)

I know we disable some stuff in backgroundPageThumbsContent.js :: init, but I don't see any existing docshell flag to suppress window.open, and don't know offhand if there's an equivalent way. (But I think we do some way to force new windows to open in a tab instead of a window -- maybe that would be useful here?)

Comment 3

[Drew Willcoxon :adw]

Yeah, ideally this wouldn't happen but I'm not sure off the top of my head how to prevent it.  I probably won't be able to look at this super soon.  CC'ing Mark in case he has any ideas.

Comment 4

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

I think the flag LOAD_FLAGS_ALLOW_POPUPS is what we are after, but I'm not sure how to go about ensuring that is set, and also not convinced this will work in all cases (eg, popups in an iframe) - eg, see bug 1279568 where we seem to be having problems with LOAD_ANONYMOUS not always being propagated correctly.

Comment 5

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

(In reply to Mark Hammond [:markh] from comment #4)
> I think the flag LOAD_FLAGS_ALLOW_POPUPS is what we are after

(to be clear, we want to ensure that's *not* set - that flag is in EXTRA_LOAD_FLAGS in nsDocShellLoadTypes.h which implies it is used as the default in at least some cases, but I'm not sure how that flag actually interacts with the rest of the world)

Comment 6

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
This sounds pretty bad, flagging for tracking.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

That particular pushset was just a table rename as far as I can tell, along with some corresponding migration code. I can't imagine why this would cause the failure in this patch.

I don't know how else I can really help out. I am not very familiar with the loading logic in nsDocShell.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Ehsan can you recommend someone (I'm bugging you as the reviewer of the patch that caused the regression.)
I agree we should fix this but it seems unlikely for the beta 49 timeframe.

Comment 9

[(no longer active)]

I asked Michael to take a look!

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1279285 - Disable window.open in the thumbnail service,

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

Comment 11

[(no longer active)]

MozReview is busted (bug 1296865) so here is my review comment:

I checked the disableDialog() API, and it was added in bug 856977 in order to prevent dialogs (meaning alert, confirm and prompts) from showing up, not popups.  Making that flag suddenly change the behavior of the browser in the onbeforeunload event case is not desirable, so this isn't the right way to fix this bug.  Also it looks like the thumbnailing code is well aware of this: <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#46>.

I think a better way would be to add a flag to nsIDocShell called allowPopups (similar to other allow flags there) and make nsGlobalWindow::PopupWhitelisted() return false when that flag is set (it should take priority over checking the popup permission for the current URI.)  Essentially that API would allow us to prevent a docshell from ever opening up a popup, and it gets hooked up to our existing popup blocking infrastructure.  Once the flag is added, you should set it from here: <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#31>

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8782592 [details]
Bug 1279285 - Disable window.open in the thumbnail service,

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

Comment 13

[(no longer active)]

Comment on attachment 8782592 [details]
Bug 1279285 - Disable window.open in the thumbnail service,

https://reviewboard.mozilla.org/r/72702/#review72092

I think the below summarizes everything I have to say about the patch, but I'd also like Boris to take a pass at it.  Please ask him for the next review.  From my perspective, r+ on a patch that addresses the below!

Thanks!

::: docshell/base/nsIDocShell.idl:306
(Diff revision 2)
>    [infallible] attribute boolean allowContentRetargetingOnChildren;
>  
>    /**
> +   * True if this docshell is allowed to create popups
> +   */
> +  attribute boolean allowPopups;

Can you please make this [infallible]?

::: dom/base/nsGlobalWindow.cpp:11728
(Diff revision 2)
>    }
>  
>    NS_ASSERTION(mDocShell, "Must have docshell here");
>  
> +  bool allowPopups = false;
> +  if (NS_SUCCEEDED(mDocShell->GetAllowPopups(&allowPopups)) && !allowPopups) {

See the condition around checkForPopup below.  Some of the conditions there specify the cases where a real popup window will be open.  For example, if the WindowExists() call returns true we will reuse that window instead of opening a new window.  I think in such cases we don't want to look at allowPopups.

The rest of the conditions in checkForPopup seem to not apply to this case (since we'd for example still want to block popups opened from a chrome page, etc.)

::: dom/base/nsGlobalWindow.cpp:11730
(Diff revision 2)
>    NS_ASSERTION(mDocShell, "Must have docshell here");
>  
> +  bool allowPopups = false;
> +  if (NS_SUCCEEDED(mDocShell->GetAllowPopups(&allowPopups)) && !allowPopups) {
> +    NS_WARNING("Window.open disabled by docshell flag");
> +    return aDoJSFixups ? NS_OK : NS_ERROR_FAILURE;

Why are you dealing with aDoJSFixups here?  That flag is only useful for popup blocking.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8782592 [details]
Bug 1279285 - Disable window.open in the thumbnail service,

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8782592 [details]
Bug 1279285 - Disable window.open in the thumbnail service,

https://reviewboard.mozilla.org/r/72702/#review72462

::: docshell/base/nsIDocShell.idl:304
(Diff revision 3)
>     * Setting allowContentRetargeting also overwrites this value.
>     */
>    [infallible] attribute boolean allowContentRetargetingOnChildren;
>  
>    /**
> +   * True if this docshell is allowed to create popups

Subject to popup blocking restrictions, right?

::: dom/base/nsGlobalWindow.cpp:11727
(Diff revision 3)
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
>    NS_ASSERTION(mDocShell, "Must have docshell here");
>  
> +  if (!mDocShell->GetAllowPopups() && !WindowExists(aName, !aCalledNoScript)) {

Is there a reason we can't use the existing sandboxing mechanism here?

In fact, is there a reason we're not already using it?  Do we allow thumbnails to do things like XHR?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8782592 [details]
Bug 1279285 - Disable window.open in the thumbnail service,

https://reviewboard.mozilla.org/r/72702/#review72464

Comment 17

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Comment on attachment 8782592 [details]
> Bug 1279285 - Disable window.open in the thumbnail service,
> 
> https://reviewboard.mozilla.org/r/72702/#review72462
> 
> ::: docshell/base/nsIDocShell.idl:304
> (Diff revision 3)
> >     * Setting allowContentRetargeting also overwrites this value.
> >     */
> >    [infallible] attribute boolean allowContentRetargetingOnChildren;
> >  
> >    /**
> > +   * True if this docshell is allowed to create popups
> 
> Subject to popup blocking restrictions, right?

Correct, I'll add a note to the comment

> 
> ::: dom/base/nsGlobalWindow.cpp:11727
> (Diff revision 3)
> >      return NS_ERROR_NOT_AVAILABLE;
> >    }
> >  
> >    NS_ASSERTION(mDocShell, "Must have docshell here");
> >  
> > +  if (!mDocShell->GetAllowPopups() && !WindowExists(aName, !aCalledNoScript)) {
> 
> Is there a reason we can't use the existing sandboxing mechanism here?
> 
> In fact, is there a reason we're not already using it?  Do we allow
> thumbnails to do things like XHR?

Yes, thumbnails can almost certainly do things like XHR.

I'm not the biggest fan of the current model for rendering thumbnails, where we render them if we don't have an image cached when the page is loaded. IMO we should not perform network requests in the background to render images for the page which people usually immediately leave to go somewhere.

That being said, that is a different bug - right now we do in fact load pages in a background process in order to take a screenshot of them for the new tab page if we don't have a screenshot cached, and sandboxing them would probably do weird things to the screenshots which we take. This patch is just aiming to get rid of the weirdness of popups appearing when the new tab page is opened.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Yes, thumbnails can almost certainly do things like XHR.

That's horrible.  Do you mind filing a bug on this?

But my overall question remains: why do we want to add a new docshell flag instead of just sandboxing this stuff to not allow popups using sandbox flags?  We could allow all the other stuff, I guess...

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Attachment: Set the SANDBOXED_AUXILARY_NAVIGATION flag when rendering thumbnails in the background

That's a good question...

Here is a 1000x simpler patch, for your reviewing pleasure.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8786352 [details] [diff] [review]
Set the SANDBOXED_AUXILARY_NAVIGATION flag when rendering thumbnails in the background

Yeah, this is more like it.  Two nits:

1)  "AUXILIARY', not "AUXILARY".
2)  Please also add SANDBOXED_MODALS so the page can't alert() and the like.

Also please file a followup bug on adding more flags here; I expect we want to at least have SANDBOXED_FORMS and SANDBOXED_TOPLEVEL_NAVIGATION, and possibly also the pointer/orientation lock and presentation bits...

r=me

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Attachment: Set the SANDBOXED_AUXILARY_NAVIGATION flag when rendering thumbnails in the background

MozReview-Commit-ID: 4grdy6evhrb

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8786500 [details] [diff] [review]
Set the SANDBOXED_AUXILARY_NAVIGATION flag when rendering thumbnails in the background

r=me, but please do get that followup bug filed.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/247a05cd8240
Set the SANDBOXED_AUXILARY_NAVIGATION flag when rendering thumbnails in the background, r=bz

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/247a05cd8240

Comment 25

[Nazir Ahmed Sabbir [:NaSb]]

I've managed this issue on this bug in Nightly 50.0a1 (2016-06-09) ; (Build ID: 20160609130607) from Ubuntu (16.04 Bit) 

This Bug is now verified as fixed on Latest Firefox Aurora 51.0a2 (2016-09-21) (64-bit)

Build ID: 20160921004005
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
OS: Linux 4.4.0-36-generic ; Ubuntu 16.04.1 (64 Bit)

Comment 26

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 50.0a1 (2016-06-09) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Aurora

Build ID   : 20160921004005
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
[bugday-20160921]

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to fix in 50.1.0 release