56296 - [RFE] Pref to disable target=_blank

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, enhancement, P3)

Description

[Greg Miller]

It'd be very nice to have an option to prevent web sites from opening windows on
my computer without my permission. target=_blank is the most common offender.

Comment 1

[Eric Pollmann]

I like that suggestion.  :)

Comment 2

[Akkana Peck]

That would be great (and is also discussed in bug 29346).

Comment 3

[Greg Miller]

Clarification: I said "disable" but I really should have said "treat it as
target=_top". Otherwise, framed sites would be even more annoying :)

Comment 4

[Akkana Peck]

Here's a patch to put it in docshell, on a preference
"browser.disallowPopupWindows".  It doesn't apply to chrome or resource urls, so
dialogs and new windows can still come up; it doesn't override middleclick or
the context menu to bring up a new window explicitly.  I made it change _blank
and _new to _top since I agree with gmiller's comment.  Reviews and opinions
solicited.

Comment 5

[Akkana Peck]

Attachment: patch in docshell

Comment 6

[Ben Bucksch (:BenB)]

s/user_pref/pref/

Otherwise, looks... simple! :-)

Comment 7

[Akkana Peck]

Adding buster: Steve, would you be the right person to super-review this?

Comment 8

[buster]

I'm not the worst choice, I guess.
1) please send a note out to [e-mail] to solicit more feedback.
2) in this code:
+      char* scheme = 0;
+      aURI->GetScheme(&scheme);
+      if (nsCRT::strcmp(scheme, "chrome") && +          nsCRT::strcmp(scheme,
"resource"))
+      {
+          static char* top = "_top";
+          aWindowTarget = top;
+      }
+      nsMemory::Free(scheme);

A) you must check "scheme" for null before passing it to strcmp() or Free()
B) I don't know if nsMemory::Free() is the right de-allocator or not, so this is
just a nag to double-check.

Comment 9

[Akkana Peck]

Thanks for looking at it, and for the suggestions.
A) nsCRT::strcmp does check its arguments for null, which I was relying on.  I
have no objection to adding an extra null test, though, in case the
implementation of nsCRT ever changes.
B) GetScheme uses nsCRT::strdup.  Turns out that there's an nsCRT::free with a
CRTFREEIF macro.  I'll change it to use CRTFREEIF, so regardless of the
implementation it'll be right (unless the URI class changes :-).

Comment 10

[Akkana Peck]

Attachment: New patch incorporating buster's suggestions

Comment 11

[Akkana Peck]

Attachment: Default pref value (forgot to attach earlier)

Comment 12

[buster]

it's too bad we need to allocate a string here at all.  I wish you could just
get a const ref to the string you need.  Maybe suggest that to whoever owns the
interface?

Comment 13

[Eric Pollmann]

If you need another reviewer, you can mark this r=pollmann@netscape.  I've
looked over the changes and it all looks good to me!

Akkana, if you would like to take this bug so when it is fixed it goes on your
list, please be my guest.  :)  (FWIW, I've applied the patch and will report
back if I notice anything out of the ordinary - great work!)

Comment 14

[Akkana Peck]

Thanks!  Yes, I'll take the bug since it'll make it easier to query (I keep
losing the bug number).

Comment 15

[Daniel Veditz [:dveditz]]

Please excuse my interjection of pref name quibbles.

- you aren't disabling "popups", at least not all of them and especially not 
the most hated ones that happen by themselves (i.e. window.open()). You're 
disabling the targeting of a new window in a link.

- prevailing pref style is to use underscores to separate word parts, not 
StudlyCaps

- forms with "enable" are preferred to "disable" 34 to 5; there are a further 9 
"allow" prefs and no "disallow".

maybe something like:
  browser.target_new.enabled
  browser.target_new_allowed

Are there going to be more prefs like this? If so maybe we should invent 
another level to group these under (and if you do that I'd be less averse to 
starting the specific node with the generic enable/disable/allow )
  browser.spamgard.allow_target_new

Comment 16

[Akkana Peck]

Interjection excused, and you're probably right about the word "popup".  I have
no objection to changing the pref name.  I'd lean toward
browser.target_new_allowed over the other one because browser.target_new.
implies that there might be other prefs relating to target_new, and I can't
imagine what they would be.  Anyone have any objection to that pref name?

There are already other spam-guarding prefs, but they're scattered to the wind
because they're potentially used for other things as well (like the JS
capability prefs, and the animated image pref, and the various cookie prefs).

Comment 17

[Jesse Ruderman]

Would this work under capability.policy.annoyances, like the pref akkana 
mentioned in bug 29346?

By the way, shouldn't the pref redirect all cases where a link opens in a new 
window to the top of the current window, and not just _new and _blank?  It's 
trivial to set each link on a page to a different target.

Comment 18

[Akkana Peck]

I agree, I'd like to see what someone suggested in bug 29346, and block any
target that doesn't have a window currently open.  

That's harder, though, so I was hoping to get this weaker fix in first.

Note, the pref name browser.target_new_allowed still fits since the stronger
behavior would still look for any new target (as opposed to targets matching the
string "_new").  Though browser.new_target_allowed might be a little clearer.

The capability.* prefs are an elaborate and general JS mechanism and I don't
think this pref should get mixed up with them even if it has behavior vaguely
similar to one of the capability prefs.

Comment 19

[Brendan Eich [:brendan]]

+  if (mDisallowPopupWindows &&
+      (!nsCRT::strcmp(aWindowTarget, "_blank") ||
+       !nsCRT::strcmp(aWindowTarget, "_new")))
+  {
+      char* scheme = 0;
+      aURI->GetScheme(&scheme);

       nsXPIDLCString scheme;
       rv = aURI->GetScheme(getter_Copies(scheme));

and maybe check rv?

+      if (scheme &&
+          nsCRT::strcmp(scheme, "chrome") &&
+          nsCRT::strcmp(scheme, "resource"))
+      {
+          static char* top = "_top";
+          aWindowTarget = top;

Why burn four bytes on a static char *?  If you want a string constant, use
const char top[], not a mutable pointer.  But if there is only one use of
"_top", there is no gain in making a string constant.  Just use "_top".

+      }
+      CRTFREEIF(scheme);

It seems best to me to use nsXPIDLCString, but if not, then move the nsCRT::free
inside the then statement governed by just if (scheme) {...}.

It is too bad that we can't test URI scheme without making a copy.  Two years
ago in porkjockeys, I recall we talked about a hasScheme method into which one
could pass a string owned by the caller.  Cc'ing Necko buddies.

Is there a different way that we could distinguish chrome: and resource: URIs
here?  Cc'ing hyatt.

/be

+  }
+

Comment 20

[Doug Turner (:dougt)]

hasScheme? what would it look like?  

something like, in C++:

nsResult nsIURI::IsScheme(const char* urlScheme, PRBool **retval)?

Comment 21

[Brendan Eich [:brendan]]

IDL first:

    boolean hasScheme(in string scheme);

C++ generated by xpidl (modulo cosmetics):

    NS_IMETHOD HasScheme(const char *aScheme, PRBool *_retval);

I suggested 'has' rather than 'is', because 'is' seems too close to OOP is-a,
and implies an equivalence relation -- but maybe I'm splitting hairs.

/be

Comment 22

[Darin Fisher]

if i recall, gagan was preparing a patch for this... there are lots of places
where we are forced to do the extra string copy.  i don't know if there is
already a bug open for this change... gagan?

Comment 23

[Gagan]

I already have these changes sitting with me for the longest time... I had of 
course used isScheme and I agree that hasScheme makes more sense. I will clean 
that up and check in the changes soon. I had another bug which talked only about 
optimizing the scheme compares but I can't find it now. 

Comment 24

[Akkana Peck]

Attachment: Patch incorporating Brendan's suggestions

Comment 25

[Akkana Peck]

The CRTFREEIF(scheme) call was where it was because otherwise there would have
been a memory leak in the chrome/resource case.  But the nsXPIDLCString model
sounds better, so I've changed it to that, removed the CRTFREEIF, and eliminated
the static char*.

When gagan's new API goes in, we should change this code to use it and eliminate
the copy, but I hope we can check this one in in the meantime.  Do we have a bug
number for the scheme checking API?  There are a few other calls which should
also be changed when that becomes available.

Comment 26

[Brendan Eich [:brendan]]

Sorry to pick nits, I'll stop after this round:

+  if (mDisallowPopupWindows &&
+      (!nsCRT::strcmp(aWindowTarget, "_blank") ||
+       !nsCRT::strcmp(aWindowTarget, "_new")))
+  {
+      nsXPIDLCString scheme;

Switching from two-space to four-space indentation(c-basic-offset)?  Modeline
violation alert!

+      nsresult rv = aURI->GetScheme(getter_Copies(scheme));
+      if (NS_SUCCEEDED(rv) && scheme &&

I believe scheme must be non-null (or convert to a non-null char pointer, I
mean) if NS_SUCCEEDED(rv).  Robustification can be appropriate when dealing with
buggy implementations, but it leads to overcomplicated, obfuscated code.  I say
one test here should suffice -- Necko guys school me if I'm wrong.

+          nsCRT::strcmp(scheme, "chrome") &&
+          nsCRT::strcmp(scheme, "resource"))
+      {
+          aWindowTarget = "_top";
+      }
+  }
+

(BTW, my argument against CRTFREEIF last time was that you could split the if
condition, making 'if (scheme) { if (...) {...}; nsCRT::free(scheme); }' to
minimize the logic.  But nsXPIDLCString rules.)

/be

Comment 27

[Gagan]

I agree. We don't want to hold this bug hostage for my checkins to go in. Since 
it is being tracked in a separate bug, I'd say go ahead with this one. Whenever 
I find the bug number I will add it here. 

Comment 28

[Akkana Peck]

Four-space indentation: I agree with you in principle, but it's how the rest of
that method (and most of the methods around it) are formatted, and I always try
to adapt to the existing formatting of files I don't own.  Won't it hurt
readability to format this clause inconsistently with the rest of the routine? 
Should I reformat the whole method?  (To 4-space tabs, to be consistent with the
emacs mode line?)  I'll do whatever you think best; I'm format agnostic as long
as you don't tell me to use tabs. :-)

Not checking scheme: it makes me nervous to assume things like that.  The last
time I removed a null check based on a review comment that it could never
happen, it haunted me for many months and at least three crash bugs.  I'll take
it out if you really want me to, but only if I can add a comment saying
something like "Necko group [or Brendan?] says this can never be null" so people
know where to assign the bugs if the impossible happens.

Comment 29

[Daniel Veditz [:dveditz]]

"supposedly redundant" checks is what assert/PR_ASSERT/NS_ASSERTION/etc are 
for, right? :-)

Comment 30

[Brendan Eich [:brendan]]

No tabs, period.

I don't see other functions using two spaces for the first tabstop, then
indenting by four space units.  Rather, I see lots of travis three-space
insanity, but we're all trying to get rid of that.  Who owns nsDocShell.cpp now?
mscott?

I think the modeline should establish a norm that patches then implement,
piecemeal or all at once.  Don't keep doing random indentation just because
someone who no longer works on Mozilla did.

What dveditz said.  If GetScheme returns null with NS_OK, it's a bug.  If
GetScheme returns a garbage pointer, there's no defense.  Practically speaking,
we have time to shake out bad impls with assertions.

/be

Comment 31

[Akkana Peck]

Okay.  In fact, the null check turns out not to matter because nsCRT doesn't do
anything bad if its argument is null.  I retract my opposition and will remove
the null check.  I didn't put an assert in because I'd have to rewrite the logic
of the if clause to do that and the failure mode turns out not to be a problem.
 And I'll indent the whole new clause consistently with four spaces (in line
with the emacs mode line and most of the indentation in the file).

Comment 32

[Akkana Peck]

Attachment: New patch: reformat, remove a null check

Comment 33

[Gagan]

For those interested, I created bug 66577 since I couldn't find the orig one.
This new bug deals with the scheme comparison changes.

Comment 34

[Akkana Peck]

In those patches I never changed the pref name.  How about
"browser.target_new_blocked" (instead of _allowed, so as not to change the logic
and to make the default false)?  Then the only changes needed to the patch are
the two lines, in all.js and nsDocShell::Create(), which reference the pref name.

Comment 35

[Daniel Veditz [:dveditz]]

The proposed prefname browser.target_new_blocked sounds fine

Comment 36

[Akkana Peck]

I couldn't resist (and it turned out to be easy): I added the extra code to
disable all target= if it refers to a window that doesn't exist.  It's only a
small addition (though the logic has been changed slightly to accomodate the new
clause), and I couldn't stand all the warnings any more (there were some new
ones today), so I also made some warning fixes for all the warnings that were
fixable without changing logic (I'll file a bug on the remaining warnings).

Comment 37

[Akkana Peck]

Attachment: Complete patch with new functionality: disable target for any new window

Comment 38

[Akkana Peck]

BTW, it seems like a bummer that FindItemWithName requires a unicode string when
it's doing most of it's comparing against ascii strings using
EqualsWithConversion, so all of its callers have to convert to unicode just so
that the strings can be converted back at compare time.  It's awfully tempting
to change it to use ascii.  Who owns docshell and might be able to rule on this?
 Should I file a separate bug?

Comment 39

[Brendan Eich [:brendan]]

mscott and rpotts know docshell.

In the classic codebase, window (target) names were constrained to be ASCII,
IIRC -- maybe even alphanumeric, with no underscores anywhere.  Anyone remember?

/be

Comment 40

[Akkana Peck]

Filed bug 66746 on the FindItemWithName issue.  It's a bug either way.

Comment 41

[Brendan Eich [:brendan]]

Now that you mention "_top" twice, may as well label a static const char array
holding those bytes.  Do that, and r/sr=[e-mail], whichever helps.

/be

Comment 42

[Daniel Veditz [:dveditz]]

r=dveditz for the 1/26 patch. Changing FindItemByName() doesn't belong in this 
patch, it's a separate issue.

Comment 43

[Akkana Peck]

Fix is in!  And I updated the customizing document with the new pref.

Comment 44

[Mike Shaver (:shaver emeritus)]

I realize that this is fixed, but I was wondering if we might do some
unification of like items here and have the target-opens-new-window behaviour
keyed off the same security policy item as the window.open DOM-manipulation control?

Maybe people want it to be different, but I've always thought that it should be
"content can't open new windows", rather than "just disable window.open for
DOM-manipulating things".

Comment 45

[Jesse Ruderman]

I want those to be different prefs (because I'd have window.open disabled and 
target="_blank" enabled), but I would like them to sit next to each other in 
the prefs wnidow.

Comment 46

[Chris Petersen]

Marking verified per last comments.

Comment 47

[Jesse Ruderman]

See also bug 70990, "need 'open in the same window' option in context menu."

Comment 48

[Jesse Ruderman]

*** Bug 39401 has been marked as a duplicate of this bug. ***

Comment 49

[Frank Tobin]

It looks like this preference is being renamed to
browser.block.target_new_window, from Bugzilla Bug 78037
  [RFE] Ability to prevent link from opening in new window.

Comment 50

[Jesse Ruderman]

*** Bug 240581 has been marked as a duplicate of this bug. ***