176166 - nsIDocShell::appType is not easily extensible

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Darin Fisher]

nsIDocShell::appType is not easily extensible.  some of our security code is app
specific (e.g., javascript disabled by default in mailnews).  code inside
nsScriptSecurityManager checks the docshell's appType attribute.  since other
apps probably care about security too, it seems likely that other apps would
want to be able to likewise define themselves as an nsIDocShell::appType, and
then modify code (perhaps inside caps) to taylor moz security for their app. 
but this is not an option for apps that are not part of the mainstream mozilla
release.

Comment 1

[Adam Lock]

It's hard to see why apptype even exists.

What's wrong with testing what the docshell supports via the various "allow"
attributes, e.g. allowJavascript, allowImages, allowSubframes, allowPlugins etc.?

I only see only potentially useful place where it's called in nsGlobalWindow.cpp

http://lxr.mozilla.org/seamonkey/search?string=etAppType

If the attribute has to live on then it could always be turned into a string to
be more flexible.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Right, you are, Adam... The current users of GetAppType are:

nsImgManager.cpp -- this should use GetAllowImages() (and mailnews needs to
                    handle the pref stuff itself in
                    nsMsgWindow::SetRootDocShell)
nsScriptSecurityManager.cpp -- this should use GetAllowJavascript (again,
                               mailnews needs to handle the prefs)
nsGlobalWindow.cpp -- this may need a new boolean flag -- "expose as opener" or
                      something.  Maybe this could become a bitfield if we end
                      up with too many flags?
nsXMLElement.cpp -- this is checking whether auto-links are allowed... perhaps
                    this should just use the meta refresh boolean?

That's all the callers.  I think we should just convert them to use the booleans
Adam pointed out and nix the apptype thing.

Comment 3

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/b556a1f684ed

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Nothing like resurrecting a 16 year old bug...

I'm currently converting nsIDocShell const lists to CEnums in bug 1505601, and just ran into APP_TYPE. AFAICT, in m-c we never set AppType to anything but APP_TYPE_UNKNOWN unless it's in tests, and APP_TYPE_MAIL is pretty much invalid for m-c now anyways. It seems like we could remove this.

However, c-c still uses APP_TYPE heavily, so removing this from nsIDocShell is going to cause them some friction.

:bz, any thoughts on how to handle this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

As you note, c-c relies on appType pretty heavily.  That's the only reason it ever existed, in fact.

Is there a need to remove it, as opposed to just switching to a CEnum?  If so, seems like we need to have  some  way to indicate the same information...

Comment 6

[Boris Zbarsky [:bzbarsky]]

That said, does c-c ever _read_ the appType?  If it just sets it, we can look at the very small number of places in m-c that it gets read and come up with some other ways for c-c to get that behavior.  That's sort of what comment 2 was about, I think.   It's been a while. ;)

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

c-c reads it once, from what I can tell. If we came up with another place to put it, we could just convert that too.

Either way, for now I'm just converting it to a cenum, and will add a "do not delete" comment there, while leaving this bug open. I'm not really sure trying to get all of the required work synced between gecko and c-c teams would be worth the effort right now. I was mostly curious about how we should deal with an issue of split usage between the repos like this, and wanted to make sure there's notes in the code so someone else doesn't try to clean it up.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(and it's switching to CEnum in bug 1505601)