[New Tab Page] thumbnail service opens a popup

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: philipp, Assigned: Nika)

Tracking

(Blocks 1 bug, {regression})

42 Branch
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49+ wontfix, firefox50 wontfix, firefox51 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Blocks: 1185343
Reporter

Updated

3 years ago
Summary: [New Tab Page] thumbnail service opens opens a popup → [New Tab Page] thumbnail service opens a popup
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.
Flags: needinfo?(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?)
Flags: needinfo?(dolske) → needinfo?(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.
Flags: needinfo?(adw)
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.
(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)
[Tracking Requested - why for this release]:
This sounds pretty bad, flagging for tracking.
Assignee: nobody → michael
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.
Assignee: michael → nobody
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

3 years ago
I asked Michael to take a look!
Assignee: nobody → michael
Flags: needinfo?(ehsan)
Comment hidden (mozreview-request)

Comment 11

3 years ago
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>

Updated

3 years ago
Attachment #8782592 - Flags: review?(ehsan) → review-
Comment hidden (mozreview-request)

Comment 13

3 years ago
mozreview-review
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.
Attachment #8782592 - Flags: review?(ehsan) → review-
Comment hidden (mozreview-request)

Comment 15

3 years ago
mozreview-review
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

3 years ago
mozreview-review
Comment on attachment 8782592 [details]
Bug 1279285 - Disable window.open in the thumbnail service,

https://reviewboard.mozilla.org/r/72702/#review72464
Attachment #8782592 - Flags: review?(bzbarsky)
(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.
> 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...
That's a good question...

Here is a 1000x simpler patch, for your reviewing pleasure.
Attachment #8786352 - Flags: review?(bzbarsky)
Assignee

Updated

3 years ago
Attachment #8782592 - Attachment is obsolete: true
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
Attachment #8786352 - Flags: review?(bzbarsky) → review+
Assignee

Updated

3 years ago
Attachment #8786352 - Attachment is obsolete: true
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.
Attachment #8786500 - Flags: review?(bzbarsky) → review+
Assignee

Updated

3 years ago
Blocks: 1299558

Comment 23

3 years ago
Pushed by michael@thelayzells.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/247a05cd8240
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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)
QA Whiteboard: [bugday-20160921]
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]
Too late to fix in 50.1.0 release
Reporter

Updated

2 years ago
See Also: → 1386288
You need to log in before you can comment on or make changes to this bug.