Giorgio Maone [:ma1] Reporter
	Description • 11 years ago

The "delete previous word" shortcut ([Ctrl-Backspace]) doesn't work in content textareas unless JavaScript is enabled.

Steps to reproduce:

1) Set the javascript.enabled about:config preference to false
2) Open any page containing a textarea (es. data:text/html,<textarea>some text</textarea> )
3) If the textarea is empty, type some words
4) Position the caret at the end of any word
5) Press [Ctrl-Backspace] on the keyboard

Expected result: the word preceding the caret gets deleted (it does if JavaScript is enabled).
Actual result: nothing happens.
Reprocible: always

	Antonio
	Comment 1 • 11 years ago

I cannot reproduce the error on version 26.0 in Linux x86_64
Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0

So it may occur on windows only

	Mats Palmgren (inactive)
	Comment 2 • 11 years ago

I can reproduce the problem in Nightly on Windows 8, but not in FF25.0.1 so it appears to be
a regression.

	Alice0775 White
	Comment 3 • 11 years ago

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0d477f84d533
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131112162707
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cd0e2e0ef136
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131112164406
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0d477f84d533&tochange=cd0e2e0ef136

Regressed by: Bug 840488

	Alex Keybl [:akeybl]
	Comment 4 • 11 years ago

This is a fraction of a fraction of our users, and minimal impact. We'd take an uplift, but not critical enough to track.

	Bobby Holley (:bholley) Assignee
	Comment 5 • 11 years ago

The fix for this is probably straightforward, but I don't have a windows environment. This seems to work fine on my mac (with options-delete being the shortcut instead).

One of two things might be happening:
* Whatever script binds the shortcut is failing to load because JS is disabled.
* Some action taken by the shortcut fails because JS is disabled.

If it's the latter, it should be very easy to debug. Just attach a debugger, get everything set up, and then break in nsScriptSecurityManager::ScriptAllowed. Then, do ctrl-backspace. If we hit that breakpoint, we just look at the backtrace, which probably tells us whatever we need to know.

If it's the former, it's probably harder.

Giorgio, are you able to run this in a debugger?

Ehsan, are you aware of the mechanism that makes this work, and if so, any ideas why it might break when we disable JS?

	Alice0775 White
	Comment 6 • 11 years ago

In local build,
Last Good: 0d477f84d533
First Bad: 9bff006f8fda
Triggered by:
	9bff006f8fda	Bobby Holley — Bug 840488 - Refactor Gecko to provide a more direct API to ask whether script is allowed for a given global. r=bz

	Giorgio Maone [:ma1] Reporter
	Comment 7 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #5)

> Giorgio, are you able to run this in a debugger?

Nope, I'm afraid I cannot, not soon at least, sorry.

	(no longer active)
	Comment 8 • 11 years ago

This is how Ctrl+Backspace is implemented: <http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/win/platformHTMLBindings.xml#50>  I'm not sure what the effect of disabling JS on XBL content is, but this sounds like a bad regression.

	Alice0775 White
	Comment 9 • 11 years ago

Strange thing is here.

It works if you follow the following STR.
1. Set the javascript.enabled about:config preference to false
2. Bookmark data url
   data:text/html,<textarea>some text</textarea>
3. Open about:home in new tab (middle click home button)
4. Open the bookmark

	Giorgio Maone [:ma1] Reporter
	Comment 10 • 11 years ago

(In reply to Alice0775 White from comment #9)
> Strange thing is here.


If you've got NoScript installed, that's the effect of an usability feature, unrelated to this bug.

	Alice0775 White
	Comment 11 • 11 years ago

No, I do not install any add-ons

	Bobby Holley (:bholley) Assignee
	Comment 12 • 11 years ago

Yeah, this is bad. We should track it. Patches coming up.

	Bobby Holley (:bholley) Assignee
	Comment 15 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=b1d1c01b453e

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 11 years ago

Comment on attachment 8344106 [details] [diff] [review]
Allow scripts for an XBL binding if and only if the XBL document comes from a scriptable domain. v1

The reason the code was the way it was is that if you have some random style in your email that attaches an XBL binding, you do not want that to be able to run script in the email.  Even if the binding comes from a "scriptable domain", no?

What might make more sense is to check whether the binding's principal subsumes the bound element.  If it does, allow script based on where the binding was loaded from.  If not, allow script based on whether the element's document allows it.  Thoughts?

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

Comment on attachment 8344107 [details] [diff] [review]
Tests. v1

>+    SpecialPowers.pushPrefEnv({ set: [['javascript.enabled', true]] }, loadFrame);

Should be "false" instead of "true", right?

	Bobby Holley (:bholley) Assignee
	Comment 18 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #16)
> The reason the code was the way it was is that if you have some random style
> in your email that attaches an XBL binding, you do not want that to be able
> to run script in the email.  Even if the binding comes from a "scriptable
> domain", no?

Hm. What principal does the email document use? Scripts already aren't allowed to apply cross-origin XBL bindings (except for ones that come from chrome), so the only bindings they'd be able to apply would be chrome bindings (unless I'm missing something).

The issue here, in my mind, is that we implement content functionality with privileged XBL, and we want that stuff to work, even if scripting is disabled for the content. See all the bugs that depend on bug 834697.
 
> What might make more sense is to check whether the binding's principal
> subsumes the bound element.

Given that scripts aren't allowed to apply cross-origin bindings (if I understand the test coverage in [1] correctly), when would this be false?

[1] http://mxr.mozilla.org/mozilla-central/source/content/xbl/test/test_bug379959.html?force=1#84

	Bobby Holley (:bholley) Assignee
	Comment 19 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #17)
> Comment on attachment 8344107 [details] [diff] [review]
> Tests. v1
> 
> >+    SpecialPowers.pushPrefEnv({ set: [['javascript.enabled', true]] }, loadFrame);
> 
> Should be "false" instead of "true", right?

Facepalm. Thanks for catching that. I'd flipped it at the end while I was fiddling with my test and must have accidentally committed that. Plus one for actually reviewing tests. :-)

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 11 years ago

> What principal does the email document use? 

Something somewhat nonce-like, iirc.

> and we want that stuff to work, even if scripting is disabled for the content

Sure, hence my suggestion that if the XBL principal subsumes the bound element principal we base the scripting decision on the XBL.  That would make the case you describe work, right?

> when would this be false

Hmm.  That's a good question.  I guess I'm mostly viewing this as a belt-and-suspenders thing.  But maybe it's safe enough to just assume that our other stuff works as advertised and XBL linked from a document always subsumes it.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 11 years ago

Comment on attachment 8344107 [details] [diff] [review]
Tests. v1

r=me with the boolean fixed

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 11 years ago

Ok, so I'm trying to understand what IsScriptEnabled() will do on a binding document.  It'll do:

7268   nsCOMPtr<nsIScriptGlobalObject> globalObject = do_QueryInterface(GetInnerWindow());

That better return null, right?  So IsScriptEnabled() will actually always return false on such a document, no?

	Bobby Holley (:bholley) Assignee
	Comment 23 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #22)
> That better return null, right?  So IsScriptEnabled() will actually always
> return false on such a document, no?

You're totally right. Duh. This is presumably why the try push in comment 15 (which I hadn't gotten around to checking until just now) is busted.

(In reply to Boris Zbarsky [:bz] from comment #21)
> r=me with the boolean fixed

As it turns out, this was actually an older version of the tests, which included some domain policy stuff which actually _whitelisted_ the mochi.test:8888 domain after disabling script globally. So the only reason the same-origin binding didn't run was because of the bug above. doh!

Sorry for the multiple layers of broken here. :-( All the weirdness around XBL tends to get me mixed up. New patches coming soon.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 11 years ago

Comment on attachment 8344784 [details] [diff] [review]
Allow scripts for an XBL binding if and only if the XBL document comes from a scriptable domain. v2

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 11 years ago

Comment on attachment 8344785 [details] [diff] [review]
Tests. v4

>+    // Disable javascript, make an exception for example.com, and load the frame.

We're not making an exception anymore.  Is the comment off, or the code?

	Bobby Holley (:bholley) Assignee
	Comment 28 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #27)
> Comment on attachment 8344785 [details] [diff] [review]
> Tests. v4
> 
> >+    // Disable javascript, make an exception for example.com, and load the frame.
> 
> We're not making an exception anymore.  Is the comment off, or the code?

The comment. Fixed locally.

	Bobby Holley (:bholley) Assignee
	Comment 29 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=28ead3b5ec1d

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 11 years ago

Comment on attachment 8344785 [details] [diff] [review]
Tests. v4

r=me

	Bobby Holley (:bholley) Assignee
	Comment 31 • 11 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd9d5186e741
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c9d48f28a5e

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 11 years ago

Backed out for mochitest-other orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b84aac1794b

https://tbpl.mozilla.org/php/getParsedLog.php?id=31760246&tree=Mozilla-Inbound

	Bobby Holley (:bholley) Assignee
	Comment 33 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=e6e856a0b564

	Bobby Holley (:bholley) Assignee
	Comment 34 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=4ff93a11ebf7

	Bobby Holley (:bholley) Assignee
	Comment 35 • 11 years ago

OK great, managed to actually hit the failure for once in comment 34.

Looks like CAPS is claiming that script is enabled on the domain, even though (earlier) the pref service claims that JS is disabled. The two possibilities are:

(A) The pref observer for CAPS is getting delayed somehow.
(B) There's a domain policy active for some reason.

I added more diagnostics, and pushed to see if I can trigger another failure:

https://tbpl.mozilla.org/?tree=Try&rev=8d2b77e654da

	Bobby Holley (:bholley) Assignee
	Comment 36 • 11 years ago

Ahah! So it appears that the pref is actually getting reset to the default (and ScriptSecurityPrefChanged is called) _before_ the second constructor runs. So I think the following is happening:

(1) The |allow| constructor runs, and triggers SimpleTest.finish().

(2) SimpleTest.finish() goes about its many-stage process of tearing things down. Multiple round-trips through the event loop are performed after resetting the prefs to the default.

(3) Sometime after the prefs are reset, but before the test completes and the page is unloaded, the |deny| binding finally gets applied (we can tell this from the log, because the nsXBLDocumentInfo is created _after_ CAPS reset mAllowJavaScript to true). Scripts are allowed at this point, so we run the constructor.

bz, can you think of any reason why we'd wait so darn long to apply the binding, especially since it's the second binding in the list (and especially since we have the 100ms delay between completing the |allow| constructor and invoking SimpleTest.finish())? Moreover, how do you think we should rejigger the test? Applying the bindings programmatically won't work, because script is disabled. Are the constructors guaranteed to run before onload is fired on the window? If so, we could have the constructors wait on that to do the meat of their work. If not, how long should we wait to let the binding run if it's ever going to?

This also explains why I couldn't reproduce the failure by increasing the 100ms delay. The failure happens when the delay is _shorter_.

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 11 years ago

> can you think of any reason why we'd wait so darn long to apply the binding

http server gremlins?  Note that it's being loaded over an http:// URI.

> especially since it's the second binding in the list 

Doesn't matter.  chrome:// bindings are loaded sync.  http:// ones are loaded async, of course.

> and especially since we have the 100ms delay between completing the |allow| constructor
> and invoking SimpleTest.finish()

This part is more confusing, but maybe something caused the scheduler to not run any of this stuff for 100ms for some reason?

> Are the constructors guaranteed to run before onload is fired on the window?

Hmm.

nsBindingManager::AddToAttachedQueue will call nsBindingManager::PostProcessAttachedQueueEvent which will block onload.  And of course the binding load itself will block onload.  We can make sure we do is to flush layout in a <script> after the divs so that we'll be sure they have frames and hence have kicked off the binding load.

The weak link is that the binding load completing will do a RecreateFramesFor, which is an async operation now and does not itself block onload.

However that operation will happen when the refresh driver ticks.  So I think it should be safe to wait until onload, then wait two refresh driver ticks (via requestAnimationFrame) and assume that constructors must have run by then.

	Bobby Holley (:bholley) Assignee
	Comment 38 • 11 years ago

Let's give that a shot on try:

https://tbpl.mozilla.org/?tree=Try&rev=a1b296ff8570

	Bobby Holley (:bholley) Assignee
	Comment 40 • 11 years ago

These are green so far. I'll give it a few more retriggers, but in the mean
time let's get bz to review the new tests so that this can all land when that
comes in.

	Boris Zbarsky [:bzbarsky]
	Comment 41 • 11 years ago

Comment on attachment 8346850 [details] [diff] [review]
Tests. v7

r=me

	Bobby Holley (:bholley) Assignee
	Comment 42 • 11 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/78b36c1db4e3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/99c9b164945e

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/78b36c1db4e3
https://hg.mozilla.org/mozilla-central/rev/99c9b164945e

	therube
	Comment 44 • 11 years ago

Is this meant to be or will it be fixed on Aurora?
Otherwise there will be an entire cycle, mozilla28, where this is broken.

And if not I'd like to request an uplift.

	Bobby Holley (:bholley) Assignee
	Comment 45 • 11 years ago

Comment on attachment 8344784 [details] [diff] [review]
Allow scripts for an XBL binding if and only if the XBL document comes from a scriptable domain. v2

(In reply to therube from comment #44)
> Is this meant to be or will it be fixed on Aurora?
> Otherwise there will be an entire cycle, mozilla28, where this is broken.

Yes, I was waiting for a tracking decision, but I'll just request uplift.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 840488
User impact if declined: various bits of browser functionality broken when script is diabled.
Testing completed (on m-c, etc.): Baked on m-c 
Risk to taking this patch (and alternatives if risky): Medium-Low Risk. We should take the patch.

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/c80a5475fb25
https://hg.mozilla.org/releases/mozilla-aurora/rev/e73905e8ca8b