1412824 - Refactor MaybeCloseWindow to allow closing the window if toplevel data URI navigation is blocked

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

Copying over the comment from bz from Bug 1403814#c36:

> Can we even fix that within nsDSURIContentListener?

Yes.  We could factor out the logic from http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/uriloader/exthandler/nsExternalHelperAppService.cpp#1591-1598,1608-1619 and http://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2526-2560 into a helper "close this window async and return its opener if it says it was just opened" thing...

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1412824_refactor_maybeclosewindow.patch

Hey Smaug, I refactored the window closing functionality from nsExternalHelperAppService. At the moment I just dumped the refactored class "nsMaybeCloseWindowService" within nsExternalHelperAppService, which on the one hand makes it easier for you to review but on the other hand it's not the right place.

Where should we put the classe nsMaybeCloseWindowService?

Other than that, the code is ready for review. I manually verified that it's working correctly and closes the blocked window.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8924536 [details] [diff] [review]
bug_1412824_refactor_maybeclosewindow.patch

>+nsIInterfaceRequestor* nsMaybeCloseWindowService::MaybeCloseWindow()
> {
I don't understand why this method returns nsIInterfaceRequestor*

>+class nsMaybeCloseWindowService final : public nsITimerCallback
Oddly named class. This isn't any service, there can be multiple instances of the class.
And drop ns-prefix.
Perhaps MAybeCloseWindowHelper

>+{
>+public:
>+  NS_DECL_THREADSAFE_ISUPPORTS
Why threadsafe refcounting?

The new class could be close to nsDocShell I think.


(IMO most of uriloader/* could be moved to closer to docshell, but others may have other opinions.)

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1412824_refactor_maybeclosewindow.patch

Really appreciate the quick turnaround time -thanks.

(In reply to Olli Pettay [:smaug] from comment #2)
> >+nsIInterfaceRequestor* nsMaybeCloseWindowService::MaybeCloseWindow()
> > {
> I don't understand why this method returns nsIInterfaceRequestor*

I tried to capture the original implementation as precisely as possible, see [1]. So, we are closing the provided window or return it's opener if the window was just opened. I added a comment explaining the behavior above MaybeCloseWindow().

[1] https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2537

> The new class could be close to nsDocShell I think.

I don't think it's worth adding a new file, but I totally agree that it should be close to nsDocShell. Since the class is now named 'MaybeCloseWindowHelper', I think it makes sense that nsDSURIContentListener hosts that class.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8924690 [details] [diff] [review]
bug_1412824_refactor_maybeclosewindow.patch

>+void MaybeCloseWindowHelper::SetShouldCloseWindow(bool aShouldCloseWindow)
Nit, void should be on its own line

>+nsIInterfaceRequestor* MaybeCloseWindowHelper::MaybeCloseWindow()
As should other return types.

> nsDSURIContentListener::~nsDSURIContentListener()
>@@ -88,16 +148,26 @@ nsDSURIContentListener::DoContent(const 
>   if (aOpenedChannel) {
>     aOpenedChannel->GetLoadFlags(&loadFlags);
> 
>     // block top-level data URI navigations if triggered by the web
>     if (!nsContentSecurityManager::AllowTopLevelNavigationToDataURI(aOpenedChannel)) {
>       // logging to console happens within AllowTopLevelNavigationToDataURI
>       aRequest->Cancel(NS_ERROR_DOM_BAD_URI);
>       *aAbortProcess = true;
>+      // close the window since the navigation to a data URI was blocked
>+      if (mDocShell) {
>+        nsCOMPtr<nsIInterfaceRequestor> contentContext =
>+          do_QueryInterface(mDocShell->GetWindow());
>+        if (contentContext) {
>+          mMaybeCloseWindowHelper = new MaybeCloseWindowHelper(contentContext);
>+          mMaybeCloseWindowHelper->SetShouldCloseWindow(true);
>+          mMaybeCloseWindowHelper->MaybeCloseWindow();

Why we need mMaybeCloseWindowHelper member variable on nsDSURIContentListener?
Just having a local RefPtr<MaybeCloseWindowHelper> 
and then calling MaybeCloseWindow() should be enough.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #4)
> >+          mMaybeCloseWindowHelper = new MaybeCloseWindowHelper(contentContext);
> >+          mMaybeCloseWindowHelper->SetShouldCloseWindow(true);
> >+          mMaybeCloseWindowHelper->MaybeCloseWindow();
> 
> Why we need mMaybeCloseWindowHelper member variable on
> nsDSURIContentListener?
> Just having a local RefPtr<MaybeCloseWindowHelper> 
> and then calling MaybeCloseWindow() should be enough.

Ha, indeed :-)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb29f6c93a3
Refactor MaybeCloseWindow and allow to reuse the window close code from within nsExternalHelperAppService as well as nsDSURIContentListener. r=smaug

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9eb29f6c93a3