Closed Bug 944407 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- unaffected
firefox28 + fixed
firefox29 + fixed

People

(Reporter: mao, Assigned: bholley)

References

Details

(Keywords: regression, reproducible, Whiteboard: [qa-])

Attachments

(2 files, 3 obsolete files)

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
Summary: Textarea's "delete previous word" shortct [Ctrl-Backspace] does not work if JavaScript is disabled → Textarea's "delete previous word" shortcut [Ctrl-Backspace] does not work if JavaScript is disabled
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
I can reproduce the problem in Nightly on Windows 8, but not in FF25.0.1 so it appears to be
a regression.
Priority: -- → P4
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
This is a fraction of a fraction of our users, and minimal impact. We'd take an uplift, but not critical enough to track.
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?
Flags: needinfo?(g.maone)
Flags: needinfo?(ehsan)
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
(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.
Flags: needinfo?(g.maone)
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.
Assignee: nobody → bobbyholley+bmo
Flags: needinfo?(ehsan)
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
(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.
No, I do not install any add-ons
Yeah, this is bad. We should track it. Patches coming up.
Summary: Textarea's "delete previous word" shortcut [Ctrl-Backspace] does not work if JavaScript is disabled → XBL scriptability should not depend on the scriptability of the bound content
Attached patch Tests. v1 (obsolete) — Splinter Review
Attachment #8344107 - Flags: review?(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 on attachment 8344107 [details] [diff] [review]
Tests. v1

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

Should be "false" instead of "true", right?
(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
(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. :-)
> 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 on attachment 8344107 [details] [diff] [review]
Tests. v1

r=me with the boolean fixed
Attachment #8344107 - Flags: review?(bzbarsky) → review+
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?
(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.
Blocks: 948000
Attachment #8344106 - Attachment is obsolete: true
Attachment #8344107 - Attachment is obsolete: true
Attachment #8344106 - Flags: review?(bzbarsky)
Attachment #8344784 - Flags: review?(bzbarsky)
Attached patch Tests. v4 (obsolete) — Splinter Review
Attachment #8344785 - Flags: review?(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
Attachment #8344784 - Flags: review?(bzbarsky) → review+
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?
(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 on attachment 8344785 [details] [diff] [review]
Tests. v4

r=me
Attachment #8344785 - Flags: review?(bzbarsky) → review+
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
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_.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Let's give that a shot on try:

https://tbpl.mozilla.org/?tree=Try&rev=a1b296ff8570
Duplicate of this bug: 949569
Attached patch Tests. v7Splinter Review
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.
Attachment #8344785 - Attachment is obsolete: true
Attachment #8346850 - Flags: review?(bzbarsky)
Comment on attachment 8346850 [details] [diff] [review]
Tests. v7

r=me
Attachment #8346850 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/78b36c1db4e3
https://hg.mozilla.org/mozilla-central/rev/99c9b164945e
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 950930
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.
Flags: needinfo?(bobbyholley+bmo)
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.
Attachment #8344784 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bobbyholley+bmo)
Whiteboard: [uplift needs dependent bug]
Attachment #8344784 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.