280120 - Turning off js in help doesn't work properly

Status: RESOLVED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Stefan [:stefanh]]

Turning off Javascript in Help by adding the line
helpBrowser.docShell.allowJavascript = false; in help.js doesn't seem to work
properly.

It works if Javascript is enabled in Preferences --> Advanced --> Script &
Plug-ins. But turning off Javascript in preferences seem to enable it in Help.

How to repro:

1. Turn off js in Help by adding helpBrowser.docShell.allowJavascript = false;
in help.js.

2. Add a script in the welcome page (welcome.xhtml)

3. Start Mozilla and turn off js in preferences (Preferences --> Advanced -->
Script & Plug-ins)

4. Notice that the script works.

Comment 1

[Stefan [:stefanh]]

Attachment: testcase-diff

Here's a patch that illustrates the problem:

Apply the patch, turn off js in preferences and open Help (Help Contents in
Help menu)

Comment 2

[[e-mail]]

CCing people who might have a clue about how this works (or doesn't...)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Looks like if JS is disabled the security manager special-cases non-system
chrome: uris and doesn't check with the docshell (see
nsScriptSecurityManager::CanExecuteScripts).

That's probably wrong, but I'm not sure what the right behavior is, exactly. 
Should docshells be able to _enable_ script if it's disabled globally and they
explicitly set the flag  to true, say?

Comment 4

[[e-mail]]

Attachment: Remove chrome: special case

The comment refers to about: URLs, but they aren't tested for. What the code
actually does is override the docShell setting for chrome: URLs but only when
javascript is globally disabled. Do we actually need that special case? Or do
we still want it, but for about: URLs rather than chrome: URLs?

Comment 5

[Boris Zbarsky [:bzbarsky]]

about: URLs redirect to chrome, so this code used to be testing for them.  But
now that chrome: ends up using originalURI, I think this test could be safely
switched to about: instead of chrome:.

Comment 6

[[e-mail]]

As it stands that will break Editor because it currently uses about:blank for a
new page and that the code block in question overrides the docShell setting.

The only benefit that I can see of allowing about: pages to run scripts is for
about: itself so that it can show the UA when JavaScript is disabled.

Comment 7

[Boris Zbarsky [:bzbarsky]]

At least that, yes.  I guess the other about: pages that run JS have system
principals....  Is about: broken right now, btw, with JS disabled?

Comment 8

[Stefan [:stefanh]]

(In reply to comment #7)
> Is about: broken right now, btw, with JS disabled?

Yes

Comment 9

[Boris Zbarsky [:bzbarsky]]

So either we need to special-case just about: (URL, not scheme) here, or we need
to come up with a more flexible setup for this stuff for about: (scheme) urls.

Comment 10

[Asa Dotzler [:asa]]

maybe for beta2.

Comment 11

[Boris Zbarsky [:bzbarsky]]

This is a really simple bug to fix; we just need to decide what approach to
take.  Note that not only is help broken, so is "about:"....

sicking, peterv, any ideas?

Bumping severity to major, since this is effectively breaking our security model
in some cases.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So to be precise, we want to:

1)  Not mess with whether JS is enabled in chrome
2)  Enable JS in "about:" (only, not other URIs with that scheme, especially not
    about:blank).

If someone edits "about:" that will give the editor conniptions, but fixing that
would require some new api to communicate "this document should ignore JS
disabling".

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 14

[[e-mail]]

Comment on attachment 174486 [details] [diff] [review]
Proposed patch

We can worry about that when editor learns xhtml.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Actually... biesi, would this break error pages?

If so, perhaps the right answer is to keep the chrome check (in addition to the
"about" check), and just put them both where I put the "about" check, after we
check for the docshell locking out script.

Of course that will cause error pages to not work in docshells that disable
script....

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

hm, yes, it seems it would :-/

maybe error pages need an own scheme...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Either that, or we need some other way of telling which principals shouldn't
have script disabling apply to them...

Comment 18

[[e-mail]]

(In reply to comment #15)
>Actually... biesi, would this break error pages?
By my limited understanding error pages currently have chrome:// content URLs
and I therefore assume system principal which is already specially cased.

Comment 19

[Boris Zbarsky [:bzbarsky]]

That's the question... do error pages have system principals?