944407 - XBL scriptability should not depend on the scriptability of the bound content

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P4)

Description

[Giorgio Maone [:ma1]]

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

Comment 1

[Antonio]

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

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Alice0775 White]

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

Comment 4

[Alex Keybl [:akeybl]]

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

Comment 5

[Bobby Holley (:bholley)]

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?

Comment 6

[Alice0775 White]

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

Comment 7

[Giorgio Maone [:ma1]]

(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.

Comment 8

[(no longer active)]

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.

Comment 9

[Alice0775 White]

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

Comment 10

[Giorgio Maone [:ma1]]

(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.

Comment 11

[Alice0775 White]

No, I do not install any add-ons

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

Attachment: Allow scripts for an XBL binding if and only if the XBL document comes from a scriptable domain. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

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

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

Comment 18

[Bobby Holley (:bholley)]

(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

Comment 19

[Bobby Holley (:bholley)]

(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. :-)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

r=me with the boolean fixed

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

[Bobby Holley (:bholley)]

(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.

Comment 24

[Bobby Holley (:bholley)]

Attachment: Allow scripts for an XBL binding if and only if the XBL document comes from a scriptable domain. v2

Comment 25

[Bobby Holley (:bholley)]

Attachment: Tests. v4

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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?

Comment 28

[Bobby Holley (:bholley)]

(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.

Comment 29

[Bobby Holley (:bholley)]

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

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 31

[Bobby Holley (:bholley)]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Bobby Holley (:bholley)]

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

Comment 34

[Bobby Holley (:bholley)]

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

Comment 35

[Bobby Holley (:bholley)]

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

Comment 36

[Bobby Holley (:bholley)]

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_.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 38

[Bobby Holley (:bholley)]

Let's give that a shot on try:

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

Comment 40

[Bobby Holley (:bholley)]

Attachment: Tests. v7

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.

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 42

[Bobby Holley (:bholley)]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[therube]

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.

Comment 45

[Bobby Holley (:bholley)]

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.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

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