77227 - Have to open new window after disabling javascript to see <noscript> content.

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Eric Pollmann]

This is a fairly minor regression due to our checkin for bug 69455.

Scenario:
  Open a page with a <noscript> tag (testcase coming shortly)
  Go to prefs->advanced
  Uncheck "Enable Javascript for Navigator"
  Click reload

Expected result:
  The content inside the <noscript> tag should show.

Actual result:
  Content inside <noscript> does not show.

In order to vew the <noscript> content, you must open a new window and load the
document in that window.

Problem description:
  The checkin moved the check of the 'javascript.enabled' preference from the
nsHTMLContentSink::OpenJavaScript method to nsDocShell::Create.  This means that
is will only be checked once per window, instead of once per document load, and
changes to the pref will not get noticed until a new window is opened.

Solution:
  Move the check of the 'javascript.enabled' preference back from
nsDocShell::create to nsHTMLContentSink::OpenJavaScript.  Patch coming...

Comment 1

[harishd]

Correction:
  Eric: The check for javascript.enabled should happen in 
nsHTMLContentSink::Init :-)

Comment 2

[Eric Pollmann]

Attachment: testcase

Comment 3

[Eric Pollmann]

Attachment: patch 1, move pref check from docshell to content sink

Comment 4

[harishd]

patch looks good. r=harishd.

Comment 5

[Brendan Eich [:brendan]]

Style nit o' the day:

+    if (enabled) {
+      mFlags |= NS_SINK_FLAG_SCRIPT_ENABLED;
+   }

The closing brace needs another space before it.  I'm happy otherwise -- any
layout guys to sr=?  Otherwise, sr=[e-mail].

/be

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Doesn't this break nsDocShell::GetAllowJavascript?  Shouldn't you either leave
the pref call in nsDocShell or call SetAllowJavascript on the docshell (or both)?

Comment 7

[Eric Pollmann]

> Doesn't this break nsDocShell::GetAllowJavascript?  Shouldn't you either leave
> the pref call in nsDocShell or call SetAllowJavascript on the docshell (or
> both)?

I am reverting the part of my patch that changed GetAllowJavascript entirely.
Actually, I broke things by checking in what I did - this puts things back to
the way they were.

There are only two callers of GetAllowJavascript:

nsScriptSecurityManager::CanExecuteScripts()
 This function will default to the value of the pref "javascript.enabled" if
 script is not disabled explicitly on this docshell via that
 Get/SetAllowJavascript APIs


HTMLContentSink::Init()
 This is the callsite that I changed that caused this bug report to be opened.
 Since were were caching the pref value in the docshell after my checkin, when
 the pref was changed, it would not take effect in currently open windows, just
 new ones.


The semantic was:

  prefs->Get/SetBoolPref("javascript.enabled");
    This is a global preference for all docshells, and should take effect
    immediately.

  nsDocShell::Get/SetAllowJavascript()
    This can be used to disable javascript for this docshell, for it's lifetime,
    and overrides the value set in the pref.

My checkin that broke things changed the pref semantic to:

  prefs->Get/SetBoolPref("javascript.enabled");
    This is a global preference for all docshells, and should takes effect
    as new docshells are created.

This patch changes it back to the old semantic.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

OK, makes sense.  Although isn't there an existing bug (that you're not really
changing) that means you won't display NOSCRIPT content if someone sets
nsIDocShell::allowJavascript to false (or causes
nsScriptSecurityManager::CanExecuteScripts to return false in some other way)?

Comment 9

[Eric Pollmann]

I've opened bug 77390 for this issue.  It is really a tiny change, but I'll
separate it from this patch to minimize risk (and hopefully get this in for 0.9)

Comment 10

[chris hofmann]

a=drivers

Comment 11

[Eric Pollmann]

Fix checked in.  To verify, open the attached testcase.  You should see:

Here is some script content:
script content
Here is some noscript content:

Go to Edit->Preferences...->Advanced and uncheck "Enable Javascript for Navigator"
Click OK to close the preferences dialog and click reload on the testcase window.
You should see:

Here is some script content:
Here is some noscript content:
noscript content

Thanks!

Comment 12

[Hixie (not reading bugmail)]

Just curious -- I know we've been doing lots of work on reducing page loading time
recently, so how expensive is this? Were jrgm's tests run with and without?

Comment 13

[Eric Pollmann]

True, it might be expensive - but prior to our last minute <noframes> change,
this is the way it was always done, so it should not have any new negative
impact on performance.

When we switch this over to using the security manager, which is an observer of
preference changes rather than fetching the pref each time, it should actually
improve performance.  See bug 77390 for more on that.  (Almost ready with a
patch there too...)

Comment 14

[Chris Petersen]

Marking verified in May 24th build.