519928 - Enable JavaScript and Plugins inside IFRAMEs in designmode="on" documents

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Paul Stone]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)
Build Identifier: 

IFRAMEs inside designMode documents have JavaScript and plugins disabled. This is a problem because currently the only protection Firefox has against clickjacking is for sites to use JavaScript-based framebusting methods. The result is almost like IE's <iframe security=restricted>. The difference is that in IE, 'security=restricted' prevents cookies from being sent, which prevents most clickjacking type attacks from working.

Although this is kind of a dupe of bug 326600, I'm not suggesting that JavaScript be enabled for IFRAMEs in designMode documents, as that would lead to other security problems for sites that use designMode.

The problem is that until Firefox implements either bug 493857 (Content Security Policy) or bug 475530 (X-Frame-Options) there's no real solution for clickjacking protection, except to make sites non-functional without JavaScript.
	



Reproducible: Always

Steps to Reproduce:
1. Insert IFRAME containing 3rd party side into designMode document
Actual Results:  
Site in IFRAME loads with JavaScript disabled

Expected Results:  
???

Comment 1

[Paul Stone]

Attachment: Testcase

This testcase shows two IFRAMEs, one with JavaScript enabled, the other with it disabled. You can try loading a site like Twitter into the designMode IFRAME and see that the framebusting is disabled. Using this method, it would be possible to resurrect Twitter's 'Don't Click' Clickjacking worm.

Comment 2

[Paul Stone]

After thinking about bug 520189, I think it may be OK to allow JavaScript for iframes inside designMode documents.

Comment 3

[Jesse Ruderman]

> The problem is that until Firefox implements either bug 493857 (Content
> Security Policy) or bug 475530 (X-Frame-Options) there's no real solution for
> clickjacking protection, except to make sites non-functional without
> JavaScript.

This designMode behavior is only one of about ten reasons why framebusting doesn't work as a clickjacking defense.  The parent page can halt your navigation, disable scripts in some other way, or just keep loading your page over and over.  So this bug isn't a security hole.

Comment 4

[Jesse Ruderman]

Paul Stone makes a good point that while framebusting is completely hopeless:

* Parent halts navigation.
* Parent undoes navigation.
* Parent keeps loading the victim page over and over.
* Internet connection is slowed down.

Using inline JS to hide content kinda works because the variables are not under the attacker's control:

* User might have JS disabled.
* User might have JS disabled for the framed site.
* User might have third-party JS disabled.
* User might have a ridiculous user stylesheet.
* Browser might be out of memory.

Comment 5

[Paul Stone]

Just to clarify, my point was that a common counter-clickjacking measure is for a site to hide its content (or add a non-interactive overlay), rather than break out of the frame. Many sites are opting to use this technique instead of X-Frame-Options, even in browsers that support that header (e.g Twitter, Facebook)

Although there many well known methods that prevent framebusting, this is the only technique I'm aware of that can prevent content-hiding techniques. While it's probably advisable for sites to move to declarative clickjacking protection techniques like CSP or X-F-O, that is not yet an option if they want to protect Firefox users.

Comment 6

[(no longer active)]

Do we disable js and plugins for security reasons?  Chrome doesn't seem to do this.

Comment 7

[Paul Stone]

I believe all browsers disable js inside designMode iframes. The only difference is that in Firefox, this is inherited by sub-iframes.

Comment 8

[(no longer active)]

Peter, could you please reply to comment 6 above?  Thanks!

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to comment #6)
> Do we disable js and plugins for security reasons?  Chrome doesn't seem to do
> this.

I think we copied IE (note that I didn't implement designmode).

Comment 10

[(no longer active)]

I think I'm leaning towards WONTFIXing this bug.  I don't feel comfortable with enabling js in design mode, without having an extensive analysis on its implications.  I think the right solution is to have some other mechanism to do this, like what IE seems to be doing (mentioned in comment 0).

Comment 11

[Paul Stone]

Ehsan, the problem isn't that JavaScript is disabled in designMode documents (IE does the same). It's that documents inside iframes in a designMode document have JavaScript disabled. The content of the iframes don't become editable, so it seems inconsistent that they inherit the non-JS policy.

Comment 12

[(no longer active)]

I suppose we could modify the loop below to explicitly allow js execution in iframes which are included in a designMode=on document.  Boris, would that be acceptable?

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1790

Comment 13

[Boris Zbarsky [:bzbarsky]]

That doesn't seem like a good idea if js is disabled on an ancestor for a reason other than designMode.

A side question: why do we even load subframes in designMode?

Comment 14

[(no longer active)]

(In reply to comment #13)
> That doesn't seem like a good idea if js is disabled on an ancestor for a
> reason other than designMode.

Yes, sure.  My question was, is it OK to do that _if_ js is disabled on an ancestor because of designMode?

> A side question: why do we even load subframes in designMode?

Why would we not want to do that?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> My question was, is it OK to do that _if_ js is disabled on an
> ancestor because of designMode

I _think_ so.

> Why would we not want to do that?

Precisely because of issues like this bug...

Comment 16

[(no longer active)]

(In reply to comment #15)
> > My question was, is it OK to do that _if_ js is disabled on an
> > ancestor because of designMode
> 
> I _think_ so.

OK, I'll have a patch which I'll attach here shortly.

> > Why would we not want to do that?
> 
> Precisely because of issues like this bug...

Maybe the issue was not well-thought to begin with, but we can solve these issues.  I don't think we should kill loading iframes because of it.

Comment 17

[(no longer active)]

Attachment: Patch (v1)

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What about IFRAMEs in contenteditable content? Do they never have JS disabled?

Comment 19

[(no longer active)]

(In reply to comment #18)
> What about IFRAMEs in contenteditable content? Do they never have JS disabled?

That's right.  The only thing which affects js execution is setting designMode="on" on a document.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Since designmode="on" is supposed to be the same as setting contenteditable on the <body> element, according to HTML5, why don't we just fix that spec-compliance bug by making designmode="on" not disable JS?

Comment 21

[(no longer active)]

(In reply to comment #20)
> Since designmode="on" is supposed to be the same as setting contenteditable on
> the <body> element, according to HTML5, why don't we just fix that
> spec-compliance bug by making designmode="on" not disable JS?

I'm not sure about the implications of doing that.  I asked this in comment 6, and peterv said in comment 9 that he thinks we just copied IE there.

How can we be sure that stopping to disable JS and plugins doesn't have any security side effects?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If it does, then wouldn't we have the same problem if someone uses 'contenteditable'?

Comment 23

[(no longer active)]

(In reply to comment #22)
> If it does, then wouldn't we have the same problem if someone uses
> 'contenteditable'?

I guess so, yeah.

Given that, I guess it makes sense to change this patch so that we no longer disallow plugins and js for the designMode=on case.  Do you want me to do that?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think that's the right thing to do. The only issue is if there is existing Web content that sets designmode="on" and relies on that disabling scripts and plugins. Want to check what other browsers do? If at least one of Webkit and IE does not disable scripts and plugins, I think we're OK.

Comment 25

[Paul Stone]

I just did a quick test in Firefox, IE and Chrome:

- IE disables javascript and plugins in designMode/contentEditable, but not in sub-iframes.
- Chrome allows both javascript and plugins in designMode/contentEditable
- Firefox disallows javascript and plugins in designMode (and sub-iframes) but allows both in contentEditable

I also tried out the CKEdtior and TinyMCE editors and inserted a flash applet in all three browsers. In all cases, a placeholder object was inserted instead of the actual flash object. 

If there are any regressions as a result of changing this behaviour, it's likely that it would show up in the above rich-text editors, since they both have a lot of browser-specific code to deal with inconsistencies in designMode behaviour between browsers.

Comment 26

[(no longer active)]

Paul's observations are correct.  So, I think we can start enabling plugins and js for designMode documents.

Also, note that the placeholder mentioned in comment 25 is added by custom code inside those editor widgets.  To test things in real browser implementations, simple editors like http://www.mozilla.org/editor/midasdemo/ work a lot better.

Morphing the bug accordingly, and will prepare another patch and attach it here.

Comment 27

[(no longer active)]

CCing David to make sure this doesn't have any a11y implications.

Comment 28

[(no longer active)]

Attachment: Patch (v2)

Enable js + plugins for designMode=on documents.

Comment 29

[(no longer active)]

Attachment: Patch (v2.1)

Also remove an old test which would fail with this change.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't we get rid of all usage of nsEditingSession::DisableJSAndPlugins and the code itself?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also, I would like to get explicit feedback from peterv to make sure he doesn't know of anything that will burn us here.

Comment 32

[(no longer active)]

(In reply to comment #30)
> Shouldn't we get rid of all usage of nsEditingSession::DisableJSAndPlugins and
> the code itself?

No, it is still used by nsEditingSession::MakeWindowEditable when aInteractive is false.  And this is useful for other consumers, for example, Thunderbird probably doesn't want js and plugins inside its compose window.

Comment 33

[(no longer active)]

Comment on attachment 438226 [details] [diff] [review]
Patch (v2.1)

(In reply to comment #31)
> Also, I would like to get explicit feedback from peterv to make sure he doesn't
> know of anything that will burn us here.

Sure, makes sense.

Comment 35

[Robert Stopp (textrinum)]

(In reply to comment #26)
> Paul's observations are correct.  So, I think we can start enabling plugins and
> js for designMode documents.
> 
> Also, note that the placeholder mentioned in comment 25 is added by custom code
> inside those editor widgets.  To test things in real browser implementations,
> simple editors like http://www.mozilla.org/editor/midasdemo/ work a lot better.
> 
> Morphing the bug accordingly, and will prepare another patch and attach it
> here.

I can assure that IE is not playing flash applets in documents with designmode="on" - without custom code. And I think thats OK. It must be possible to edit the object-containers by selecting them. 
It is possible to set such containers uneditable/enabled by assigning the contenteditable="false" attribute - inside a contenteditable document/ container.
Webkit is not a reference, it sends forms when clicking on a submit button...
(https://bugzilla.mozilla.org/attachment.cgi?id=337902)
The name is _designmode_ !
In my opinion iframes(contentWindows) wouldn't displayed in designmode and a placeholder would shown.

Comment 36

[Robert Stopp (textrinum)]

What happend if the user in a iframe inside an editable iframe clicks a link with target="_parent"? Sounds that like designmode/ contenteditable?

Comment 37

[David Bolter [:davidb] (NeedInfo me for attention)]

Cc'ing Alexander as this has obvious implications for the editor UI behaviour doc/spec he is drafting.

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #37)
> Cc'ing Alexander as this has obvious implications for the editor UI behaviour
> doc/spec he is drafting.

David, maybe I lost sight of the point but I don't see implications for the editor behaviour. The one thing I worry whether disabled JS affects on ARIA widgets which are JS based what leads to inaccessible content. Another issue is web richtext editors like CKEditor should continue to work, but we could ask CKEditor developers to check try build before the patch landing.

Comment 39

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #38)
> (In reply to comment #37)
> > Cc'ing Alexander as this has obvious implications for the editor UI behaviour
> > doc/spec he is drafting.
> 
> David, maybe I lost sight of the point but I don't see implications for the
> editor behaviour. The one thing I worry whether disabled JS affects on ARIA
> widgets which are JS based what leads to inaccessible content. Another issue is
> web richtext editors like CKEditor should continue to work, but we could ask
> CKEditor developers to check try build before the patch landing.

Nope, you got the point exactly :)

Comment 40

[Garry Yao]

Hi all, we at CKEditor believe enable JS in either contentEditable/designMode will just open a huge gate for security holes as well as compromise the accessibility features, considering we have the following CF_HTML on clipboard and get pasted into the midas demo:
<img onmousedown="alert('hijack');return false;" src="..." />
Now that happens when user in editing just clicks on the content - he first missed the default image resize handler, then get his editing content exposed to 3rd attacker.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So do you think Chrome and Safari already have those security holes?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note that patches in bug 520189 will sanitize HTML content pasted or drag/dropped into the editor.

Comment 43

[Schuyler Duveen]

As someone that has been waiting a long time for Bug 326600, that got marked as a dupe for this...
We'd need to un-dup Bug 326600 if enabling JS doesn't happen.
Fixing only 326600 would also fix the JS frame-jacking attempt--since you could embed an iframe into your page marked with designMode="off" and run your de-frame-jacking code.

The fixing that or this bug, would allow some very rich widgets inside wysiwyg editors--allowing the widget/sub-frame to behave like a simple readonly-image--but with clickable interactions.

Perhaps there's a way to privilege the contenteditable select over the on* attributes?

Comment 44

[Robert Stopp (textrinum)]

The spec (http://dev.w3.org/html5/spec/Overview.html#contenteditable) says and gives the way: 
"Select and move non-editable elements nested inside editing hosts: 
    UAs should offer a way for the user to move images and other non-editable parts around the content within an editing host. This may be done using the drag and drop mechanism. User agents must not, in response to a request to move non-editable elements nested inside editing hosts, generate a DOM that is less conformant than the DOM prior to the request."

So the default behaviour would be: Iframes and plugins are contenteditable, selectable and don't load.
An app or a user could decide to load the element (contenteditable="false"), Javascript should be enabled in subframes. If the element needs to be selectable for the app (or the UA) the app must overlay the element with a "Clickjacking"-trap.

Comment 45

[Garry Yao]

(In reply to comment #41)
> So do you think Chrome and Safari already have those security holes?
Note that webkit sanitizes the pasted data, while at least current Firefox clipboard implementation doesn't include such secureness.

Comment 46

[Alfonso Martinez]

If plugins are enabled inside a document that it's being edited, the user won't be able to get the context menu event on them, and they can't also select that object so once they have inserted such plugin into their documents they can't change it. IIRC, it's possible to embed a flash movie inside a powerpoint, and in that case the plugin is also disabled during the edit of the document. Another example of the need of being able to edit the current content is the fact that due to bug 446670, in CKEditor we might end up using placeholders instead of the native elements for those form items in Firefox. Far from ideal, but users need to edit what's inside the document.

In CKEditor iFrames still aren't being handled specifically, but probably in the future they will be handled like other external objects and they will be replaced with a placeholder to get the same cross-browser experience as there are some problems right now that the few people that have tried to use them have reported. Despite the fact that scripts are run or not, they might want to change its dimensions, src or other properties, so it has to be controlled from the parent page.

The overall idea of enabling Javascript in a document that it's being edited it's just useless for an editing widget. If it could be possible to let the javascript run, but magically get the DOM that matches the state without such execution (and after editing other parts of the content) then it would be great, but the reality is that in CKEditor and other WYSIWYG editors we must be sure that no javascript or event handlers are executed because even if there are times when they can be useful, there are too many other times when they would ruin the content.

Simple example:
<script> document.write("Hello")</script>

If that script is run, the first time the user will see a Hello and say "great", but when he saves the page and sees the final result he gets 2 Hellos, and that's not what he wanted.

So unless someone has a way to revert any js change done to the document that can affect the final output we must assure that such thing doesn't happen.

In the end, I don't think that this will mean a new problem with CKEditor as we are forced to deal with this situation in all the browsers (Webkit probably has the worst implementation of contentEditable across all the engines, so doing something because they do it is a bad idea), but there are many other simple scripts that might not be prepared to handle this as they are focused only in IE and Firefox and this change might force them to change their code to handle this situation.

Comment 47

[(no longer active)]

(In reply to comment #36)
> What happend if the user in a iframe inside an editable iframe clicks a link
> with target="_parent"? Sounds that like designmode/ contenteditable?

What currently happens is that the parent iframe's document is replaced with a new one.  Note that whether or not we take the fix for this bug doesn't have any effect on this issue.

Comment 48

[(no longer active)]

(In reply to comment #38)
> (In reply to comment #37)
> > Cc'ing Alexander as this has obvious implications for the editor UI behaviour
> > doc/spec he is drafting.
> 
> David, maybe I lost sight of the point but I don't see implications for the
> editor behaviour. The one thing I worry whether disabled JS affects on ARIA
> widgets which are JS based what leads to inaccessible content.

So this should be a win for accessibility, right?

Comment 49

[(no longer active)]

(In reply to comment #40)
> Hi all, we at CKEditor believe enable JS in either contentEditable/designMode
> will just open a huge gate for security holes as well as compromise the
> accessibility features, considering we have the following CF_HTML on clipboard
> and get pasted into the midas demo:
> <img onmousedown="alert('hijack');return false;" src="..." />
> Now that happens when user in editing just clicks on the content - he first
> missed the default image resize handler, then get his editing content exposed
> to 3rd attacker.

As roc pointed out in comment 42, this problem will be fixed by bug 520189.

Comment 50

[(no longer active)]

(In reply to comment #44)
> The spec (http://dev.w3.org/html5/spec/Overview.html#contenteditable) says and
> gives the way: 
> "Select and move non-editable elements nested inside editing hosts: 
>     UAs should offer a way for the user to move images and other non-editable
> parts around the content within an editing host. This may be done using the
> drag and drop mechanism. User agents must not, in response to a request to move
> non-editable elements nested inside editing hosts, generate a DOM that is less
> conformant than the DOM prior to the request."
> 
> So the default behaviour would be: Iframes and plugins are contenteditable,
> selectable and don't load.
> An app or a user could decide to load the element (contenteditable="false"),
> Javascript should be enabled in subframes. If the element needs to be
> selectable for the app (or the UA) the app must overlay the element with a
> "Clickjacking"-trap.

Firstly, you can't use contenteditable="false" in designMode documents.  Example:

data:text/html,<script>document.designMode="on";</script>test <span contenteditable="false">me</span> now

The other issue is that currently, by disabling plugins, we're not providing a placeholder for them which can be dragged, moved, or resized.  We just don't show anything on the screen.

Comment 51

[(no longer active)]

(In reply to comment #46)
> If plugins are enabled inside a document that it's being edited, the user won't
> be able to get the context menu event on them, and they can't also select that
> object so once they have inserted such plugin into their documents they can't
> change it.

Yes, but the situation with plugins disabled is exactly the same!  The user can't select the plugin, get the context menu, etc., because the user can't see anything on the screen.

> IIRC, it's possible to embed a flash movie inside a powerpoint, and
> in that case the plugin is also disabled during the edit of the document.
> Another example of the need of being able to edit the current content is the
> fact that due to bug 446670, in CKEditor we might end up using placeholders
> instead of the native elements for those form items in Firefox. Far from ideal,
> but users need to edit what's inside the document.

Yes, I would agree that it's desirable to be able to interact with plugins in editable documents in that way, but that can still happen as part of a separate bug.  What I'm saying is that the issue is not directly related to this bug.

> In CKEditor iFrames still aren't being handled specifically, but probably in
> the future they will be handled like other external objects and they will be
> replaced with a placeholder to get the same cross-browser experience as there
> are some problems right now that the few people that have tried to use them
> have reported. Despite the fact that scripts are run or not, they might want to
> change its dimensions, src or other properties, so it has to be controlled from
> the parent page.

The same issue with iframes.  Currently we don't treat them as opaque objects which can be selected, or manipulated.

> The overall idea of enabling Javascript in a document that it's being edited
> it's just useless for an editing widget. If it could be possible to let the
> javascript run, but magically get the DOM that matches the state without such
> execution (and after editing other parts of the content) then it would be
> great, but the reality is that in CKEditor and other WYSIWYG editors we must be
> sure that no javascript or event handlers are executed because even if there
> are times when they can be useful, there are too many other times when they
> would ruin the content.
> 
> Simple example:
> <script> document.write("Hello")</script>
> 
> If that script is run, the first time the user will see a Hello and say
> "great", but when he saves the page and sees the final result he gets 2 Hellos,
> and that's not what he wanted.
> 
> So unless someone has a way to revert any js change done to the document that
> can affect the final output we must assure that such thing doesn't happen.

This is a valid concern.  Boris, do you know if we have a way to deal with that?

> In the end, I don't think that this will mean a new problem with CKEditor as we
> are forced to deal with this situation in all the browsers (Webkit probably has
> the worst implementation of contentEditable across all the engines, so doing
> something because they do it is a bad idea), but there are many other simple
> scripts that might not be prepared to handle this as they are focused only in
> IE and Firefox and this change might force them to change their code to handle
> this situation.

We're not simply doing this because Webkit is.  We're doing it for the sake of consistency with contenteditable="true" and compliance with the HTML5 spec in that regard.  Ultimately I think that the reason IE did this in the first place was to avoid security issues, which can be better solved in other ways.

Comment 52

[(no longer active)]

This bug should depend on bug 520189, so that we can make sure that we're not allowing unsafe js from sources such as the clipboard.

Comment 53

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #48)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > Cc'ing Alexander as this has obvious implications for the editor UI behaviour
> > > doc/spec he is drafting.
> > 
> > David, maybe I lost sight of the point but I don't see implications for the
> > editor behaviour. The one thing I worry whether disabled JS affects on ARIA
> > widgets which are JS based what leads to inaccessible content.
> 
> So this should be a win for accessibility, right?

Mostly the win is for everyone I think. The ARIA exposure is only improved in as much as JS will update the ARIA attributes; so yeah marginal a11y win :)

Comment 54

[Robert Stopp (textrinum)]

> Firstly, you can't use contenteditable="false" in designMode documents. 
> Example:
> 
> data:text/html,<script>document.designMode="on";</script>test <span
> contenteditable="false">me</span> now
> 
> The other issue is that currently, by disabling plugins, we're not providing a
> placeholder for them which can be dragged, moved, or resized.  We just don't
> show anything on the screen.
Gecko can't use contenteditable="false" in designMode documents!
The spec says:
If an element is editable and its parent element is not, or if an element is editable and it has no parent element, then the element is an editing host. Editable elements can be nested. User agents must make editing hosts focusable (which typically means they enter the tab order). An editing host can contain non-editable sections, these are handled as described below. An editing host can contain non-editable sections that contain further editing hosts.

The (no)Problem with Placeholders:
Simply add the the following rules to chrome://res/editorOverride.css
object{
	display:block;
	border:1px dashed #bebebe;
	background-image:(path/to/geckoObjectPlaceHolder)
}
iframe{
	display:block;
	border:1px dashed #bebebe;
	background-image:(path/to/geckoIframePlaceHolder)
}
And last: It is nice to hear what bug 520189 is about. Maybe some others have also an opinion and would like to diskuss.

Comment 55

[Robert Stopp (textrinum)]

(In reply to comment #53)

> Mostly the win is for everyone I think. The ARIA exposure is only improved in
> as much as JS will update the ARIA attributes; so yeah marginal a11y win :)

Which actions needs inline scripting because they can't handled by an external eventhandler like this one:
https://bugzilla.mozilla.org/attachment.cgi?id=337902 ?

Comment 56

[Alfonso Martinez]

(In reply to comment #54)
> Gecko can't use contenteditable="false" in designMode documents!

Yes. None of Firefox, Safari and Opera can handle that: bug 462735. And it seems that IE8 doesn't respect contentEditable=false if the document is in standards mode http://dev.fckeditor.net/ticket/5562

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #50)
> Firstly, you can't use contenteditable="false" in designMode documents. 
> Example:
> 
> data:text/html,<script>document.designMode="on";</script>test <span
> contenteditable="false">me</span> now

I believe that's simply a bug, according to the HTML5 spec.

I take comment #46 quite seriously.

I think this whole discussion should move to the whatwg mailing list or the public-html mailing list. I'll kick off a thread on whatwg.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I posted a summary of the problem to the WHATWG list, subject "Should scripts and plugins in contenteditable content be enabled or disabled?"

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we want to fix the security problem here before that thread is resolved, I think we should probably go ahead with the fix to allow scripts to run in IFRAMEs when script was disabled in the parent only because of designmode.

Comment 60

[(no longer active)]

(In reply to comment #57)
> (In reply to comment #50)
> > Firstly, you can't use contenteditable="false" in designMode documents. 
> > Example:
> > 
> > data:text/html,<script>document.designMode="on";</script>test <span
> > contenteditable="false">me</span> now
> 
> I believe that's simply a bug, according to the HTML5 spec.

Yes.  Basically our handling of designMode="on" and contenteditable="true" on the body element is broken (they should do the exact same thing).  I don't know if changing that globally is risk-free though, since it has the potential of breaking a lot of random things.

> I take comment #46 quite seriously.

I agree.

(In reply to comment #58)
> I posted a summary of the problem to the WHATWG list, subject "Should scripts
> and plugins in contenteditable content be enabled or disabled?"

Great, thanks!

(In reply to comment #59)
> If we want to fix the security problem here before that thread is resolved, I
> think we should probably go ahead with the fix to allow scripts to run in
> IFRAMEs when script was disabled in the parent only because of designmode.

Sounds to be the best option right now.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here's an interesting comment on the WHATWG list:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-May/026365.html

Ojan suggests that the right thing to do is to enable JS and plugins in editable content generally, but use sandboxed iframes to disable JS and plugins in editable documents in iframes, if that's desired:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-sandbox

That sounds good to me: simple and flexible. But I'm interested in what others think, especially Alfonso.

If people agree then I think the way to proceed is to take the temporary fix suggested in comment #59, implement <iframe sandbox>, and then revisit this bug and apply the real fix in Ehsan's patch.

Comment 62

[(no longer active)]

(In reply to comment #59)
> If we want to fix the security problem here before that thread is resolved, I
> think we should probably go ahead with the fix to allow scripts to run in
> IFRAMEs when script was disabled in the parent only because of designmode.

Can this be done by recording a flag indicating whether js is disabled because of designMode, and then checking that and not disabling js if that flag is true here? <http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2472>

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems like we need to separately track whether js is disabled because of non-designmode reasons, and whether it's disabled at all.

Comment 64

[(no longer active)]

(In reply to comment #63)
> It seems like we need to separately track whether js is disabled because of
> non-designmode reasons, and whether it's disabled at all.

Yes, sure.  What I meant to ask was is that the right part in the docshell code to mess with transferring the js execution permissions to iframes.

Comment 65

[Alfonso Martinez]

As I said in the last paragraph of comment #46, while an user is editing its contents we (CKEditor, and other editors) can't allow any javascript or event handlers from that content, it's too risky for us. So as long as we keep supporting all the current browsers, we will keep our patches to avoid that content from running.

A similar thing happens with plugins and iframes: the user must be able to select, drag, resize, change its properties... so we need to show placeholders for such content, and those placeholders are created for every browser, no matter if it disabled the elements or not.

That means that sandboxes might look nice, but they aren't available in the browsers that most of the people use and until we get rid of that heritage the system that it's already in place will remain. 

But at the same time, being able to run plugins even in editable content can be useful so people is able to see the page as it will look when published: a command could switch between using the object placeholders and the real plugins/iframes. All of that done at the editor level and the only requirement is that the browser allows to run those objects in editable content.

This situation is different for editing hosts based only in Gecko like Thunderbird, Kompozer or the new Blue Griffon as so far they might have relied on this disabled state of scripts and plugins (as they didn't need cross-browser compatibility) and then they find an unexpected problem when they update to this revision. I don't see any comment from Daniel Glazman here, so I'm not sure if he is aware of this topic.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good point.

Alfonso, I understand the legacy issue. I guess my real question for you is whether you think the solution outlined in comment #61 is a good solution to promote going forward.

Comment 67

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I have always been - and remain - very circumspect about enabling JS and plugins in editable content generally (re comment 61). I am even circumspect about allowing CSS Transitions and Animations to run in editable content. All of these can turn the editing environment in a gigantic circus, seen from a user's perspective. As an example, if JS is allowed, navigation menus based on JS will become uneditable since they will trigger navigation.

Sandboxed iframes running js inside editable content don't seem to me harmful but I did not investigate too long.

Comment 68

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #67)

> Sandboxed iframes running js inside editable content don't seem to me harmful
> but I did not investigate too long.

wait... what if the script living inside that iframe relies on data provided by a script hosted at the toplevel editable content (with JS disabled then...). In that case, the result will be unpredictable, more generally resulting in errors and the fact the iframe has or has not JS will be helpless to the rendering: horked anyway.

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually the proposal is that editors should use sandboxed iframes to disable js in their editable content.

This doesn't give editors the ability to disable JS just in contenteditable parts of a document. But we don't implement that anyway and if we did, we'd have to include support for very fine-grained enabling/disabling of JS, which gets hard to spec and implement.

I hope that editors that want to enable general editing of HTML pages can use sandboxed iframes and that editors that want to use contenteditable also want to restrict the DOM vocabulary anyway to exclude scripts and plugins.

Comment 70

[Alfonso Martinez]

Personally I'm not worried at the moment about this changes for CKEditor, Webkit does behave that way and thanks to the special processing the scripts and events don't run while a document is edited.

But the comment about sandboxes makes me think that the usefulness of this change with regards to security will be null. Unless I'm wrong, the "hurry" in this bug (compared with bug 326600 that requested the core of this long ago) is due to the risk of attackers embedding a site and thanks to the forced disabled javascript, such iframe can't do the old framebusting tricks to avoid phising and click jacking.

And suddenly the suggestion is to use a sandboxed iframe to disable javascript  when an editting widgets includes an iframe. Doesn't that means that sandbox will mean the same security risk?

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, as far as I can tell <iframe sandbox> only disables scripts in that iframe, not child iframes.

Come to think of it, that might have its own vulnerabilities. I'll get back to you.

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I'm wrong, "In addition, any browsing contexts nested within an iframe, either directly or indirectly, must have all the flags set on them as were set on the iframe's Document's browsing context when the iframe's Document was created."

So your point is valid. I'll raise it on the list.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, this is not specifically an editing problem, it's a completely general problem of a conflict between sandboxed iframes and JS-based counter-clickjacking measures. I don't really see a way around this unless we completely abandon the idea of sandboxed IFRAMEs, which would be bad IMHO.

(In reply to comment #5)
> Just to clarify, my point was that a common counter-clickjacking measure is for
> a site to hide its content (or add a non-interactive overlay), rather than
> break out of the frame. Many sites are opting to use this technique instead of
> X-Frame-Options, even in browsers that support that header (e.g Twitter,
> Facebook)

Why? Could we improve CSP or X-Frame-Options to address this?

Comment 74

[Collin Jackson]

This research paper (from W2SP 2010) explains how to counter clickjacking in a way that is robust to sandboxed IFRAMEs:

http://w2spconf.com/2010/papers/p27.pdf

Comment 75

[Paul Stone]

(In reply to comment #73)
> > Many sites are opting to use this technique instead of
> > X-Frame-Options, even in browsers that support that header (e.g Twitter,
> > Facebook)
> 
> Why? Could we improve CSP or X-Frame-Options to address this?

Maybe they believe JS methods to be fully effective, you'd have to ask them. However, I see no reason why these sites can't use both JavaScript and CSP/X-F-O together.


(In reply to comment #74)
> This research paper (from W2SP 2010) explains how to counter clickjacking in a
> way that is robust to sandboxed IFRAMEs:

The method suggested in that paper effectively breaks the site for people with JavaScript disabled. Here's what it says:

> Note that users who have JavaScript disabled, via browser setting or 
> NoScript, will not be able to use the site. Designers might want to have a
> fallback mechanism if such is the case. In our example the entire page is  
> initially invisible, but this defense can be more fine grained by having 
> sub-elements be invisible instead. This way, a user can be presented with a 
> message if JavaScript is disabled. However, enabling any subset of 
> functionality beyond that simple message is not advised.

So, it does protect against sandboxed IFRAMEs but I'm not sure it's a solution that's suitable for all sites.

Comment 76

[(no longer active)]

Comment on attachment 437974 [details] [diff] [review]
Patch (v1)

OK, I verified that this patch actually implements the behavior proposed in comment 59.

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 437974 [details] [diff] [review]
Patch (v1)

I can't really review docshell/caps.

Comment 78

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

Comment on attachment 437974 [details] [diff] [review]
Patch (v1)

>From: Ehsan Akhgari <[e-mail]>
>
>Bug 519928 - IFRAME inside designMode disables JavaScript, breaking current clickjacking defenses
>
>
>diff --git a/caps/src/nsScriptSecurityManager.cpp b/caps/src/nsScriptSecurityManager.cpp
>--- a/caps/src/nsScriptSecurityManager.cpp
>+++ b/caps/src/nsScriptSecurityManager.cpp
>@@ -1780,24 +1780,37 @@ nsScriptSecurityManager::CanExecuteScrip
> 
>     nsCOMPtr<nsIDocShellTreeItem> globalObjTreeItem =
>         do_QueryInterface(docshell);
> 
>     if (globalObjTreeItem) 
>     {
>         nsCOMPtr<nsIDocShellTreeItem> treeItem(globalObjTreeItem);
>         nsCOMPtr<nsIDocShellTreeItem> parentItem;
>+        PRBool childDocshellAllowingScripts = PR_FALSE;
> 
>         // Walk up the docshell tree to see if any containing docshell disallows scripts
>         do
>         {
>             rv = docshell->GetAllowJavascript(result);
>             if (NS_FAILED(rv)) return rv;
>-            if (!*result)
>-                return NS_OK; // Do not run scripts
>+            if (!*result) {
>+                if (childDocshellAllowingScripts) {
>+                    PRBool inDesignMode;
>+                    rv = docshell->GetInDesignMode(&inDesignMode);
>+                    // If we're in design mode, allow scripts in children
>+                    // unless they specifically prevent it.
>+                    if (!inDesignMode)
>+                        return NS_OK;
>+                } else {
>+                    return NS_OK; // Do not run scripts
>+                }
>+            }
Could you keep inDesignMode flag only in the document. It is a state or property
of a document, not docshell, IMHO.
GetAllowJavascript should return usually true, so the new virtual
GetInDesignMode call shouldn't cause any harm in normal cases.

r- because I'd like to avoid adding the boolean member to nsDocshell, since the
same information is already available in nsIDocument.

Comment 79

[(no longer active)]

Attachment: Patch (v1.1)

So, I tried to make things work using only the designMode flag on the document, but I think that it's not possible.  The problematic case (which is the fifth test in the testcase included in this patch) is when the parent docshell has js disabled for another reason (for example, some other code setting allowJavascript to false), and is also in the design mode.  In such a case, the code in CanExecuteScript cannot detect that, because normal callers using SetAllowJavascript and nsEditingSession caller both set the same flag.

I can't figure out how to solve this using only the designMode flag in the document.  Olli, do you have any ideas?

Comment 80

[(no longer active)]

Attachment: WIP

I found out that nsEditingSession stores the bit which indicates when js and plugins have been disabled because of designMode="on".  So I decided to use that, but for some reason, this patch doesn't work.

nsIDocShell::GetAllowJavascript always returns true (even for the first test, for example) inside CanExecuteScripts.

Does anyone have an idea why?

Comment 81

[(no longer active)]

I need help here rom someone who knows the docshell code...

Comment 82

[Boris Zbarsky [:bzbarsky]]

GetAllowJavascript just returns a docshell-internal boolean.  It reflects the state of SetAllowJavascript on that docshell.  Is someone calling SetAllowJavascript here?

Comment 83

[(no longer active)]

(In reply to comment #82)
> GetAllowJavascript just returns a docshell-internal boolean.  It reflects the
> state of SetAllowJavascript on that docshell.  Is someone calling
> SetAllowJavascript here?

Considering the first test in the unit test, for example, SetAllowJavascript is called only three times (the first time with true when initiating the load, the second time with false when the test code sets the property, and a third time with false (I'm not sure why the third call happens, but anyway it's called with false the third time as well):

$5 = 1
#0  nsDocShell::SetAllowJavascript (this=0x167a0220, aAllowJavascript=1) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:1874
#1  0x152bcb50 in nsDocShell::SetDocLoaderParent (this=0x167a0220, aParent=0x201fac60) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:2472
#2  0x152f500e in nsDocLoader::AddChildLoader (this=0x201fac60, aChild=0x167a0220) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:720
#3  0x152bbb9e in nsDocShell::AddChild (this=0x201fac60, aChild=0x167a02bc) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:2989
#4  0x1345b51b in AddTreeItemToTreeOwner (aItem=0x167a02bc, aOwningContent=0x215d2590, aOwner=0x201fb230, aParentType=1, aParentNode=0x201facfc) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:479
#5  0x1345eaff in nsFrameLoader::EnsureDocShell (this=0x215d2600) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:1092
#6  0x1345f1c7 in nsFrameLoader::CheckForRecursiveLoad (this=0x215d2600, aURI=0x215d2740) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:1162
#7  0x1345fbec in nsFrameLoader::CheckURILoad (this=0x215d2600, aURI=0x215d2740) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:325
#8  0x1345fcc7 in nsFrameLoader::LoadURI (this=0x215d2600, aURI=0x215d2740) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:234
#9  0x1345dbcf in nsFrameLoader::LoadFrame (this=0x215d2600) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:199
#10 0x1356da9f in nsGenericHTMLFrameElement::LoadSrc (this=0x215d2590) at /Users/ehsanakhgari/moz/src/content/html/content/src/nsGenericHTMLElement.cpp:2902
#11 0x13571f66 in nsGenericHTMLFrameElement::BindToTree (this=0x215d2590, aDocument=0x1c2de00, aParent=0x215d2460, aBindingParent=0x0, aCompileEventHandlers=1) at /Users/ehsanakhgari/moz/src/content/html/content/src/nsGenericHTMLElement.cpp:2927
#12 0x1347412b in nsINode::doInsertChildAt (this=0x215d2460, aKid=0x215d2590, aIndex=1, aNotify=0, aChildArray=@0x215d248c) at /Users/ehsanakhgari/moz/src/content/base/src/nsGenericElement.cpp:3563
#13 0x134743e9 in nsGenericElement::InsertChildAt (this=0x215d2460, aKid=0x215d2590, aIndex=1, aNotify=0) at /Users/ehsanakhgari/moz/src/content/base/src/nsGenericElement.cpp:3510
#14 0x130b639d in nsINode::AppendChildTo (this=0x215d2460, aKid=0x215d2590, aNotify=0) at nsINode.h:528
#15 0x13912523 in nsHtml5TreeOperation::Append (this=0x1c701ac, aNode=0x215d2590, aParent=0x215d2460, aBuilder=0x21528130) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5TreeOperation.cpp:207
#16 0x13912b3e in nsHtml5TreeOperation::Perform (this=0x1c701ac, aBuilder=0x21528130, aScriptElement=0xbfffc514) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5TreeOperation.cpp:245
#17 0x139189e9 in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x21528130) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5TreeOpExecutor.cpp:485
#18 0x13919cba in nsHtml5ExecutorReflusher::Run (this=0x215cfa00) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5TreeOpExecutor.cpp:90
#19 0x004a9a40 in nsThread::ProcessNextEvent (this=0x913870, mayWait=0, result=0xbfffce34) at /Users/ehsanakhgari/moz/src/xpcom/threads/nsThread.cpp:547
#20 0x00426f4d in NS_ProcessPendingEvents_P (thread=0x913870, timeout=20) at nsThreadUtils.cpp:200
#21 0x12f2985d in nsBaseAppShell::NativeEventCallback (this=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/xpwidgets/nsBaseAppShell.cpp:126
#22 0x12ed4cfd in nsAppShell::ProcessGeckoEvents (aInfo=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/cocoa/nsAppShell.mm:394
#23 0x9453615b in __CFRunLoopDoSources0 ()
#24 0x94533c1f in __CFRunLoopRun ()
#25 0x945330f4 in CFRunLoopRunSpecific ()
#26 0x94532f21 in CFRunLoopRunInMode ()
#27 0x936620fc in RunCurrentEventLoopInMode ()
#28 0x93661ded in ReceiveNextEventCommon ()
#29 0x93661d36 in BlockUntilNextEventMatchingListInMode ()
#30 0x961cd135 in _DPSNextEvent ()
#31 0x961cc976 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#32 0x9618ebef in -[NSApplication run] ()
#33 0x12ed3a4f in nsAppShell::Run (this=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/cocoa/nsAppShell.mm:747
#34 0x15b9e1e4 in nsAppStartup::Run (this=0x16a76d30) at /Users/ehsanakhgari/moz/src/toolkit/components/startup/src/nsAppStartup.cpp:192
#35 0x000127d2 in XRE_main (argc=6, argv=0xbfffeb84, aAppData=0xa004c0) at /Users/ehsanakhgari/moz/src/toolkit/xre/nsAppRunner.cpp:3624
#36 0x00002629 in main (argc=6, argv=0xbfffeb84) at /Users/ehsanakhgari/moz/src/browser/app/nsBrowserApp.cpp:158
#37 0x000022aa in start ()
$6 = 0
#0  nsDocShell::SetAllowJavascript (this=0x167a0220, aAllowJavascript=0) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:1874
#1  0x004c9173 in NS_InvokeByIndex_P (that=0x167a02b8, methodIndex=22, paramCount=1, params=0xbfff9748) at /Users/ehsanakhgari/moz/src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
#2  0x13ca2286 in CallMethodHelper::Invoke (this=0xbfff9724) at /Users/ehsanakhgari/moz/src/js/src/xpconnect/src/xpcwrappednative.cpp:3002
#3  0x13ca4984 in CallMethodHelper::Call (this=0xbfff9724) at /Users/ehsanakhgari/moz/src/js/src/xpconnect/src/xpcwrappednative.cpp:2308
#4  0x13c9eafb in XPCWrappedNative::CallMethod (ccx=@0xbfff991c, mode=XPCWrappedNative::CALL_SETTER) at /Users/ehsanakhgari/moz/src/js/src/xpconnect/src/xpcwrappednative.cpp:2272
#5  0x13cb05e0 in XPCWrappedNative::SetAttribute (ccx=@0xbfff991c) at xpcprivate.h:2568
#6  0x13ca96b1 in XPC_WN_GetterSetter (cx=0x1425000, obj=0x169e5400, argc=1, argv=0x16260218, vp=0x1626023c) at /Users/ehsanakhgari/moz/src/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1825
#7  0x00123066 in js_Invoke (cx=0x1425000, args=@0xbfff9acc, flags=2) at jsinterp.cpp:654
#8  0x001235d2 in js_InternalInvoke (cx=0x1425000, obj=0x169e5400, fval=379475040, flags=0, argc=1, argv=0xbfffa1d0, rval=0xbfffa1d0) at jsinterp.cpp:694
#9  0x001236f5 in js_InternalGetOrSet (cx=0x1425000, obj=0x169e5400, id=376052932, fval=379475040, mode=JSACC_WRITE, argc=1, argv=0xbfffa1d0, rval=0xbfffa1d0) at jsinterp.cpp:730
#10 0x00146aa3 in JSScopeProperty::set (this=0x1290cf0, cx=0x1425000, obj=0x169e5400, vp=0xbfffa1d0) at jsscope.h:1019
#11 0x001378f9 in js_NativeSet (cx=0x1425000, obj=0x169e5400, sprop=0x1290cf0, added=false, vp=0xbfffa1d0) at /Users/ehsanakhgari/moz/src/js/src/jsobj.cpp:4819
#12 0x0013aed5 in js_SetPropertyHelper (cx=0x1425000, obj=0x169e5400, id=376052932, defineHow=1, vp=0xbfffa1d0) at /Users/ehsanakhgari/moz/src/js/src/jsobj.cpp:5222
#13 0x0010d3c3 in js_Interpret (cx=0x1425000) at jsops.cpp:1827
#14 0x001230ef in js_Invoke (cx=0x1425000, args=@0xbfffa3bc, flags=0) at jsinterp.cpp:664
#15 0x001235d2 in js_InternalInvoke (cx=0x1425000, obj=0x214a5f00, fval=379495968, flags=0, argc=1, argv=0x155d810, rval=0xbfffa4c8) at jsinterp.cpp:694
#16 0x000804ae in JS_CallFunctionValue (cx=0x1425000, obj=0x214a5f00, fval=379495968, argc=1, argv=0x155d810, rval=0xbfffa4c8) at /Users/ehsanakhgari/moz/src/js/src/jsapi.cpp:4634
#17 0x13733d52 in nsJSContext::CallEventHandler (this=0x201fab90, aTarget=0x2150fd68, aScope=0x214a5f00, aHandler=0x169ea620, aargv=0x213f72b0, arv=0xbfffaaa4) at /Users/ehsanakhgari/moz/src/dom/base/nsJSEnvironment.cpp:2204
#18 0x137bf6c5 in nsJSEventListener::HandleEvent (this=0x21580c00, aEvent=0x17b110c0) at /Users/ehsanakhgari/moz/src/dom/src/events/nsJSEventListener.cpp:228
#19 0x135229d5 in nsEventListenerManager::HandleEventSubType (this=0x215cc100, aListenerStruct=0x215d1e60, aListener=0x21580c00, aDOMEvent=0x17b110c0, aCurrentTarget=0x2150fd78, aPhaseFlags=6, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventListenerManager.cpp:1094
#20 0x13522db9 in nsEventListenerManager::HandleEventInternal (this=0x215cc100, aPresContext=0x1ba6000, aEvent=0xbfffb440, aDOMEvent=0xbfffaf58, aCurrentTarget=0x2150fd78, aFlags=6, aEventStatus=0xbfffaf5c, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventListenerManager.cpp:1190
#21 0x13552e2e in nsEventListenerManager::HandleEvent (this=0x215cc100, aPresContext=0x1ba6000, aEvent=0xbfffb440, aDOMEvent=0xbfffaf58, aCurrentTarget=0x2150fd78, aFlags=6, aEventStatus=0xbfffaf5c, aPusher=0xbfffaf68) at nsEventListenerManager.h:146
#22 0x13552ff1 in nsEventTargetChainItem::HandleEvent (this=0x1994600, aVisitor=@0xbfffaf50, aFlags=6, aMayHaveNewListenerManagers=0, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventDispatcher.cpp:212
#23 0x135511bc in nsEventTargetChainItem::HandleEventTargetChain (this=0x19944e0, aVisitor=@0xbfffaf50, aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventDispatcher.cpp:341
#24 0x135520ca in nsEventDispatcher::Dispatch (aTarget=0x201fb2d0, aPresContext=0x1ba6000, aEvent=0xbfffb440, aDOMEvent=0x0, aEventStatus=0xbfffb468, aCallback=0x0, aTargets=0x0) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventDispatcher.cpp:628
#25 0x1310ffdd in DocumentViewerImpl::LoadComplete (this=0x16a4eea0, aStatus=0) at /Users/ehsanakhgari/moz/src/layout/base/nsDocumentViewer.cpp:1037
#26 0x152b6edd in nsDocShell::EndPageLoad (this=0x201fac60, aProgress=0x201fac74, aChannel=0x213bf080, aStatus=0) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:5756
#27 0x152b0ee3 in nsDocShell::OnStateChange (this=0x201fac60, aProgress=0x201fac74, aRequest=0x213bf080, aStateFlags=131088, aStatus=0) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:5637
#28 0x152f66f2 in nsDocLoader::FireOnStateChange (this=0x201fac60, aProgress=0x201fac74, aRequest=0x213bf080, aStateFlags=131088, aStatus=0) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:1321
#29 0x152f6ea2 in nsDocLoader::doStopDocumentLoad (this=0x201fac60, request=0x213bf080, aStatus=0) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:929
#30 0x152f7210 in nsDocLoader::DocLoaderIsEmpty (this=0x201fac60, aFlushLayout=1) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:805
#31 0x152f84fa in nsDocLoader::OnStopRequest (this=0x201fac60, aRequest=0x16af3050, aCtxt=0x0, aStatus=0) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:700
#32 0x12312e52 in nsLoadGroup::RemoveRequest (this=0x201fae50, request=0x16af3050, ctxt=0x0, aStatus=0) at /Users/ehsanakhgari/moz/src/netwerk/base/src/nsLoadGroup.cpp:680
#33 0x134440fb in nsDocument::DoUnblockOnload (this=0x1c2de00) at /Users/ehsanakhgari/moz/src/content/base/src/nsDocument.cpp:6944
#34 0x1344422a in nsDocument::UnblockOnload (this=0x1c2de00, aFireSync=1) at /Users/ehsanakhgari/moz/src/content/base/src/nsDocument.cpp:6886
#35 0x13433069 in nsDocument::DispatchContentLoadedEvents (this=0x1c2de00) at /Users/ehsanakhgari/moz/src/content/base/src/nsDocument.cpp:3887
#36 0x1344a6cf in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run (this=0x993d10) at nsThreadUtils.h:347
#37 0x004a9a40 in nsThread::ProcessNextEvent (this=0x913870, mayWait=0, result=0xbfffce34) at /Users/ehsanakhgari/moz/src/xpcom/threads/nsThread.cpp:547
#38 0x00426f4d in NS_ProcessPendingEvents_P (thread=0x913870, timeout=20) at nsThreadUtils.cpp:200
#39 0x12f2985d in nsBaseAppShell::NativeEventCallback (this=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/xpwidgets/nsBaseAppShell.cpp:126
#40 0x12ed4cfd in nsAppShell::ProcessGeckoEvents (aInfo=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/cocoa/nsAppShell.mm:394
#41 0x9453615b in __CFRunLoopDoSources0 ()
#42 0x94533c1f in __CFRunLoopRun ()
#43 0x945330f4 in CFRunLoopRunSpecific ()
#44 0x94532f21 in CFRunLoopRunInMode ()
#45 0x936620fc in RunCurrentEventLoopInMode ()
#46 0x93661eb1 in ReceiveNextEventCommon ()
#47 0x93661d36 in BlockUntilNextEventMatchingListInMode ()
#48 0x961cd135 in _DPSNextEvent ()
#49 0x961cc976 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#50 0x9618ebef in -[NSApplication run] ()
#51 0x12ed3a4f in nsAppShell::Run (this=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/cocoa/nsAppShell.mm:747
#52 0x15b9e1e4 in nsAppStartup::Run (this=0x16a76d30) at /Users/ehsanakhgari/moz/src/toolkit/components/startup/src/nsAppStartup.cpp:192
#53 0x000127d2 in XRE_main (argc=6, argv=0xbfffeb84, aAppData=0xa004c0) at /Users/ehsanakhgari/moz/src/toolkit/xre/nsAppRunner.cpp:3624
#54 0x00002629 in main (argc=6, argv=0xbfffeb84) at /Users/ehsanakhgari/moz/src/browser/app/nsBrowserApp.cpp:158
#55 0x000022aa in start ()
$7 = 0
#0  nsDocShell::SetAllowJavascript (this=0x215e46f0, aAllowJavascript=0) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:1874
#1  0x152bcb50 in nsDocShell::SetDocLoaderParent (this=0x215e46f0, aParent=0x167a0220) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:2472
#2  0x152f500e in nsDocLoader::AddChildLoader (this=0x167a0220, aChild=0x215e46f0) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:720
#3  0x152bbb9e in nsDocShell::AddChild (this=0x167a0220, aChild=0x215e478c) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:2989
#4  0x1345b51b in AddTreeItemToTreeOwner (aItem=0x215e478c, aOwningContent=0x21553ea0, aOwner=0x201fb230, aParentType=1, aParentNode=0x167a02bc) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:479
#5  0x1345eaff in nsFrameLoader::EnsureDocShell (this=0x16adce30) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:1092
#6  0x1345f1c7 in nsFrameLoader::CheckForRecursiveLoad (this=0x16adce30, aURI=0xa300a0) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:1162
#7  0x1345fbec in nsFrameLoader::CheckURILoad (this=0x16adce30, aURI=0xa300a0) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:325
#8  0x1345fcc7 in nsFrameLoader::LoadURI (this=0x16adce30, aURI=0xa300a0) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:234
#9  0x1345dbcf in nsFrameLoader::LoadFrame (this=0x16adce30) at /Users/ehsanakhgari/moz/src/content/base/src/nsFrameLoader.cpp:199
#10 0x1356da9f in nsGenericHTMLFrameElement::LoadSrc (this=0x21553ea0) at /Users/ehsanakhgari/moz/src/content/html/content/src/nsGenericHTMLElement.cpp:2902
#11 0x13571f66 in nsGenericHTMLFrameElement::BindToTree (this=0x21553ea0, aDocument=0x1bf1200, aParent=0x215afe70, aBindingParent=0x0, aCompileEventHandlers=1) at /Users/ehsanakhgari/moz/src/content/html/content/src/nsGenericHTMLElement.cpp:2927
#12 0x1347412b in nsINode::doInsertChildAt (this=0x215afe70, aKid=0x21553ea0, aIndex=0, aNotify=0, aChildArray=@0x215afe9c) at /Users/ehsanakhgari/moz/src/content/base/src/nsGenericElement.cpp:3563
#13 0x134743e9 in nsGenericElement::InsertChildAt (this=0x215afe70, aKid=0x21553ea0, aIndex=0, aNotify=0) at /Users/ehsanakhgari/moz/src/content/base/src/nsGenericElement.cpp:3510
#14 0x130b639d in nsINode::AppendChildTo (this=0x215afe70, aKid=0x21553ea0, aNotify=0) at nsINode.h:528
#15 0x13912523 in nsHtml5TreeOperation::Append (this=0x16a4215c, aNode=0x21553ea0, aParent=0x215afe70, aBuilder=0x21528130) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5TreeOperation.cpp:207
#16 0x13912b3e in nsHtml5TreeOperation::Perform (this=0x16a4215c, aBuilder=0x21528130, aScriptElement=0xbfff99e4) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5TreeOperation.cpp:245
#17 0x13918669 in nsHtml5TreeOpExecutor::FlushDocumentWrite (this=0x21528130) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5TreeOpExecutor.cpp:586
#18 0x138ba675 in nsHtml5Parser::ParseFragment (this=0x16a31a60, aSourceBuffer=@0xbfff9b80, aTargetNode=0x215afe70, aContextLocalName=0x16a980e0, aContextNamespace=3, aQuirks=1) at /Users/ehsanakhgari/moz/src/parser/html/nsHtml5Parser.cpp:480
#19 0x13572935 in nsGenericHTMLElement::SetInnerHTML (this=0x215afe70, aInnerHTML=@0xbfff9b80) at /Users/ehsanakhgari/moz/src/content/html/content/src/nsGenericHTMLElement.cpp:738
#20 0x13d08707 in nsIDOMNSHTMLElement_SetInnerHTML (cx=0x1425000, obj=0x169e5540, id=378778148, vp=0xbfffa1d0) at dom_quickstubs.cpp:17526
#21 0x00146b65 in JSScopeProperty::set (this=0x13d1ab0, cx=0x1425000, obj=0x169e5540, vp=0xbfffa1d0) at jsscope.h:1028
#22 0x0013aa29 in js_SetPropertyHelper (cx=0x1425000, obj=0x169e5540, id=378778148, defineHow=1, vp=0xbfffa1d0) at /Users/ehsanakhgari/moz/src/js/src/jsobj.cpp:5128
#23 0x0010d3c3 in js_Interpret (cx=0x1425000) at jsops.cpp:1827
#24 0x001230ef in js_Invoke (cx=0x1425000, args=@0xbfffa3bc, flags=0) at jsinterp.cpp:664
#25 0x001235d2 in js_InternalInvoke (cx=0x1425000, obj=0x214a5f00, fval=379495968, flags=0, argc=1, argv=0x155d810, rval=0xbfffa4c8) at jsinterp.cpp:694
#26 0x000804ae in JS_CallFunctionValue (cx=0x1425000, obj=0x214a5f00, fval=379495968, argc=1, argv=0x155d810, rval=0xbfffa4c8) at /Users/ehsanakhgari/moz/src/js/src/jsapi.cpp:4634
#27 0x13733d52 in nsJSContext::CallEventHandler (this=0x201fab90, aTarget=0x2150fd68, aScope=0x214a5f00, aHandler=0x169ea620, aargv=0x213f72b0, arv=0xbfffaaa4) at /Users/ehsanakhgari/moz/src/dom/base/nsJSEnvironment.cpp:2204
#28 0x137bf6c5 in nsJSEventListener::HandleEvent (this=0x21580c00, aEvent=0x17b110c0) at /Users/ehsanakhgari/moz/src/dom/src/events/nsJSEventListener.cpp:228
#29 0x135229d5 in nsEventListenerManager::HandleEventSubType (this=0x215cc100, aListenerStruct=0x215d1e60, aListener=0x21580c00, aDOMEvent=0x17b110c0, aCurrentTarget=0x2150fd78, aPhaseFlags=6, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventListenerManager.cpp:1094
#30 0x13522db9 in nsEventListenerManager::HandleEventInternal (this=0x215cc100, aPresContext=0x1ba6000, aEvent=0xbfffb440, aDOMEvent=0xbfffaf58, aCurrentTarget=0x2150fd78, aFlags=6, aEventStatus=0xbfffaf5c, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventListenerManager.cpp:1190
#31 0x13552e2e in nsEventListenerManager::HandleEvent (this=0x215cc100, aPresContext=0x1ba6000, aEvent=0xbfffb440, aDOMEvent=0xbfffaf58, aCurrentTarget=0x2150fd78, aFlags=6, aEventStatus=0xbfffaf5c, aPusher=0xbfffaf68) at nsEventListenerManager.h:146
#32 0x13552ff1 in nsEventTargetChainItem::HandleEvent (this=0x1994600, aVisitor=@0xbfffaf50, aFlags=6, aMayHaveNewListenerManagers=0, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventDispatcher.cpp:212
#33 0x135511bc in nsEventTargetChainItem::HandleEventTargetChain (this=0x19944e0, aVisitor=@0xbfffaf50, aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=0xbfffaf68) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventDispatcher.cpp:341
#34 0x135520ca in nsEventDispatcher::Dispatch (aTarget=0x201fb2d0, aPresContext=0x1ba6000, aEvent=0xbfffb440, aDOMEvent=0x0, aEventStatus=0xbfffb468, aCallback=0x0, aTargets=0x0) at /Users/ehsanakhgari/moz/src/content/events/src/nsEventDispatcher.cpp:628
#35 0x1310ffdd in DocumentViewerImpl::LoadComplete (this=0x16a4eea0, aStatus=0) at /Users/ehsanakhgari/moz/src/layout/base/nsDocumentViewer.cpp:1037
#36 0x152b6edd in nsDocShell::EndPageLoad (this=0x201fac60, aProgress=0x201fac74, aChannel=0x213bf080, aStatus=0) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:5756
#37 0x152b0ee3 in nsDocShell::OnStateChange (this=0x201fac60, aProgress=0x201fac74, aRequest=0x213bf080, aStateFlags=131088, aStatus=0) at /Users/ehsanakhgari/moz/src/docshell/base/nsDocShell.cpp:5637
#38 0x152f66f2 in nsDocLoader::FireOnStateChange (this=0x201fac60, aProgress=0x201fac74, aRequest=0x213bf080, aStateFlags=131088, aStatus=0) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:1321
#39 0x152f6ea2 in nsDocLoader::doStopDocumentLoad (this=0x201fac60, request=0x213bf080, aStatus=0) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:929
#40 0x152f7210 in nsDocLoader::DocLoaderIsEmpty (this=0x201fac60, aFlushLayout=1) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:805
#41 0x152f84fa in nsDocLoader::OnStopRequest (this=0x201fac60, aRequest=0x16af3050, aCtxt=0x0, aStatus=0) at /Users/ehsanakhgari/moz/src/uriloader/base/nsDocLoader.cpp:700
#42 0x12312e52 in nsLoadGroup::RemoveRequest (this=0x201fae50, request=0x16af3050, ctxt=0x0, aStatus=0) at /Users/ehsanakhgari/moz/src/netwerk/base/src/nsLoadGroup.cpp:680
#43 0x134440fb in nsDocument::DoUnblockOnload (this=0x1c2de00) at /Users/ehsanakhgari/moz/src/content/base/src/nsDocument.cpp:6944
#44 0x1344422a in nsDocument::UnblockOnload (this=0x1c2de00, aFireSync=1) at /Users/ehsanakhgari/moz/src/content/base/src/nsDocument.cpp:6886
#45 0x13433069 in nsDocument::DispatchContentLoadedEvents (this=0x1c2de00) at /Users/ehsanakhgari/moz/src/content/base/src/nsDocument.cpp:3887
#46 0x1344a6cf in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run (this=0x993d10) at nsThreadUtils.h:347
#47 0x004a9a40 in nsThread::ProcessNextEvent (this=0x913870, mayWait=0, result=0xbfffce34) at /Users/ehsanakhgari/moz/src/xpcom/threads/nsThread.cpp:547
#48 0x00426f4d in NS_ProcessPendingEvents_P (thread=0x913870, timeout=20) at nsThreadUtils.cpp:200
#49 0x12f2985d in nsBaseAppShell::NativeEventCallback (this=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/xpwidgets/nsBaseAppShell.cpp:126
#50 0x12ed4cfd in nsAppShell::ProcessGeckoEvents (aInfo=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/cocoa/nsAppShell.mm:394
#51 0x9453615b in __CFRunLoopDoSources0 ()
#52 0x94533c1f in __CFRunLoopRun ()
#53 0x945330f4 in CFRunLoopRunSpecific ()
#54 0x94532f21 in CFRunLoopRunInMode ()
#55 0x936620fc in RunCurrentEventLoopInMode ()
#56 0x93661eb1 in ReceiveNextEventCommon ()
#57 0x93661d36 in BlockUntilNextEventMatchingListInMode ()
#58 0x961cd135 in _DPSNextEvent ()
#59 0x961cc976 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#60 0x9618ebef in -[NSApplication run] ()
#61 0x12ed3a4f in nsAppShell::Run (this=0x997d60) at /Users/ehsanakhgari/moz/src/widget/src/cocoa/nsAppShell.mm:747
#62 0x15b9e1e4 in nsAppStartup::Run (this=0x16a76d30) at /Users/ehsanakhgari/moz/src/toolkit/components/startup/src/nsAppStartup.cpp:192
#63 0x000127d2 in XRE_main (argc=6, argv=0xbfffeb84, aAppData=0xa004c0) at /Users/ehsanakhgari/moz/src/toolkit/xre/nsAppRunner.cpp:3624
#64 0x00002629 in main (argc=6, argv=0xbfffeb84) at /Users/ehsanakhgari/moz/src/browser/app/nsBrowserApp.cpp:158
#65 0x000022aa in start ()

Comment 84

[(no longer active)]

Attachment: Patch (v2)

OK, please ignore comment 83.  The algorithm in CanExecuteScript didn't cover many possible cases.  I tweaked that algorithm, and everything seems to be working as expected.  I also extended the unit test to test the cases where the immediate parent or a grandparent of the iframe is put into design mode.  This should implement comment 59 without having to maintain an extra boolean on the docshell.

Comment 85

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

Comment on attachment 454384 [details] [diff] [review]
Patch (v2)

Ok, though this is a bit ugly, and I think
the code should be probably somewhere
in docshell.

Probably the whole docshell tree iteration
could go to docshell and SSM would then
just ask docshell->CanExecuteScripts().
Docshell has faster ways to access
document and editing session, so
it would be a bit faster too.
If you don't want to do that change in this
bug, please file a followup bug.

Comment 86

[Martijn Wargers (dead)]

Why do something different as IE?
I think it's a bad idea to change Mozilla's behavior (which is now similar to IE) into something that is different from IE.

Comment 87

[Boris Zbarsky [:bzbarsky]]

Might be worth avoiding all the else-after-return there, for clarity...

Comment 88

[(no longer active)]

(In reply to comment #86)
> Why do something different as IE?
> I think it's a bad idea to change Mozilla's behavior (which is now similar to
> IE) into something that is different from IE.

We're not necessarily trying to preserve IE compatibility as far as the editor is concerned (and that support is non-existent even now, FWIW).  Please see the discussions in this bug for the risks involved in the current behavior, and why this is the best way we found to fix this, considering breaking compatibility as little as possible.

Comment 89

[Martijn Wargers (dead)]

Fwiw, I've read the discussion in this bug and I still don't see why breaking IE compatibility is a good thing here. I guess the developers in question don't think this will cause problems.

Comment 90

[(no longer active)]

(In reply to comment #89)
> Fwiw, I've read the discussion in this bug and I still don't see why breaking
> IE compatibility is a good thing here. I guess the developers in question don't
> think this will cause problems.

Well, Webkit based browsers do not disable javascript and plugins in editable documents at all.  Also, according to the HTML5 spec, specifying the contenteditable property on the body element and setting the designMode property of the document object should result in the exact same behavior, which means that we should either disable js and plugins for contenteditable documents as well, or enable them for both modes.

So, web authors already need to work around all of these differences between browsers.

Nobody really knows why IE chose to disable js and plugins in designMode documents to begin with.  We suspect that it was done because of security concerns, but those concerns do not seem to be valid.  The real-world concern that web developers have, however, is that it's useful for them to have a mode in which they can count of plugins not being instantiated, and content script not modifying the DOM, etc., and we are preserving that behavior in this bug.

Comment 91

[(no longer active)]

Attachment: Patch (v2.1)

Moved the hierarchy navigation code to the docshell, and removed elses after return.

Comment 92

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

Comment on attachment 454551 [details] [diff] [review]
Patch (v2.1)


>diff --git a/caps/src/nsScriptSecurityManager.cpp b/caps/src/nsScriptSecurityManager.cpp
>--- a/caps/src/nsScriptSecurityManager.cpp
>+++ b/caps/src/nsScriptSecurityManager.cpp
>@@ -90,16 +90,18 @@
> #include "nsDOMJSUtils.h"
> #include "nsAboutProtocolUtils.h"
> #include "nsIClassInfo.h"
> #include "nsIURIFixup.h"
> #include "nsCDefaultURIFixup.h"
> #include "nsIChromeRegistry.h"
> #include "nsPrintfCString.h"
> #include "nsIContentSecurityPolicy.h"
>+#include "nsIDOMNSHTMLDocument.h"
>+#include "nsIEditingSession.h"

No need for these.

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -11161,8 +11161,100 @@ nsDocShell::GetPrintPreview(nsIWebBrowse
>   return NS_ERROR_NOT_IMPLEMENTED;
> #endif
> }
> 
> 
> #ifdef DEBUG
> unsigned long nsDocShell::gNumberOfDocShells = 0;
> #endif
>+
>+NS_IMETHODIMP
>+nsDocShell::GetCanExecuteScripts(PRBool *aResult, PRBool *aContinueLooking)
>+{
>+  NS_ENSURE_ARG_POINTER(aResult);
>+  *aResult = PR_FALSE; // disallow by default
>+
>+  nsCOMPtr<nsIDocShell> docshell = this;
>+  nsCOMPtr<nsIDocShellTreeItem> globalObjTreeItem =
>+      do_QueryInterface(docshell);
>+
>+  if (globalObjTreeItem)
>+  {
>+      nsCOMPtr<nsIDocShellTreeItem> treeItem(globalObjTreeItem);
>+      nsCOMPtr<nsIDocShellTreeItem> parentItem;
>+      PRBool firstPass = PR_TRUE;
>+      PRBool lookForParents = PR_FALSE;
>+
>+      // Walk up the docshell tree to see if any containing docshell disallows scripts
>+      do
>+      {
>+          nsresult rv = docshell->GetAllowJavascript(aResult);
>+          if (NS_FAILED(rv)) return rv;
>+          if (!*aResult) {
>+              nsDocShell* realDocshell = static_cast<nsDocShell*>(docshell.get());
>+              nsIDocument* doc = realDocshell->mContentViewer->GetDocument();
Null check mContentViewer

>+              NS_ASSERTION(doc, "The docshell should have a doc!");
>+              if (doc->HasFlag(NODE_IS_EDITABLE)) {
And don't add the assertion, but a null check here

>+                  nsCOMPtr<nsIEditingSession> editSession;
>+                  realDocshell->mEditorData->GetEditingSession(getter_AddRefs(editSession));
Is it guaranteed that mEditorData isn't null? You could null check in the
same if where you check the doc has the flag.


>+++ b/docshell/base/nsIDocShell.idl
>+
>+  /**
>+   * Whether this docshell can execute scripts based on its hierarchy.
>+   */
>+  readonly attribute boolean canExecuteScript;

Hmm, you have nsDocShell::GetCanExecuteScripts, but
canExecuteScript.

First one ends with 's'.

Comment 93

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

And please add some more documentation for the .idl attribute.
You could explain there how designMode affects to the value.

Comment 94

[(no longer active)]

Landed with all the comments addressed:

http://hg.mozilla.org/mozilla-central/rev/d1cbe16de6bf

Comment 95

[(no longer active)]

I knew that I'm going to miss something when moving between various revisions of this patch!

http://hg.mozilla.org/mozilla-central/rev/c9b9f32a152b

Comment 96

[(no longer active)]

I backed out both patches because of redness and orangeness in the tree.  I'm investigating the failures locally to figure out what happened.

Comment 97

[(no longer active)]

Attachment: Patch (v2.1)

So, it turns out that the caps code had a sneaky null check for the docshell, so I added it.  I've pushed this patch to the try server, but it works locally for a bunch of tests.

Comment 98

[Schuyler Duveen]

I tested this build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c64b82ade958/tryserver-linux64/
and it's passed all the tests here:
http://www.columbia.edu/~sky/bugs/firefox/intraIframeJS/
(i.e. several wysiwyg editors)

everyday browsing and testing with other wysiwyg editors hasn't yielded any obvious issues.

excellent!

Comment 99

[(no longer active)]

I'm so glad to head that.  Thanks for testing this, Schuyler!

Comment 100

[(no longer active)]

Comment on attachment 454613 [details] [diff] [review]
Patch (v2.1)

FWIW, this patch passed all unit tests on the try server.

Comment 101

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/43ff6c264cbd

Comment 102

[Martijn Wargers (dead)]

I don't get it. Something like this:
<script>document.designMode = 'on';setTimeout(function() {alert('test');}, 100);</script>
doesn't fire the alert. It seems like javascript is still disabled after load. Or is that a desired effect?

Comment 103

[Paul Stone]

Martijn, see comment 59. The patch here only enables JavaScript and plugins in iframes where the parent has designMode="on". JavaScript and plugins inside the designMode document itself are still disabled for now, though I believe they will be enabled in another bug. I've updated the summary to make it clear what changed in this bug.

Comment 104

[(no longer active)]

Thanks Paul for the correction!

Comment 105

[Martijn Wargers (dead)]

Wondering what that other bug is, that one is at least not fixed yet, although event listeners script is executed.