Closed Bug 1315146 Opened 8 years ago Closed 8 years ago

Input box cursor is reset to the front when offsetWidth/offsetHeight are accessed on main page and an iframe under certain circumstances.

Categories

(Core :: DOM: Core & HTML, defect)

30 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: tslater2006, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file testcase.zip (deleted) —
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36 Steps to reproduce: In the attached test case zip are 2 files, ffbug.html and ffiframe.html. In order to reproduce this behavior ffbug.html must be placed on http://domain.com and ffiframe.html on http://sub.domain.com. Open the ffbug.html in latest Firefox (Windows and Mac). Observe that typing in the textbox results in normal behavior, press the "Click here to trigger the bug" button and try typing in the textbox again. This only occurs if ffiframe.html is on a different subdomain that ffbug.html. Both files set the document.domain via javascript to match so cross frame access shouldn't be an issue. Actual results: After clicking the button an interval is set that accesses the document body's offsetWidth and offsetHeight as well as the offsetHeight of the iframe's body. The overflow is changed on the document body twice as well. While this interval is firing, the cursor position for every input on the page continously resets to the begining of the input. The results in the spelling of words like "cat" to be entered as "tac". Expected results: Altering the overflow property and accessing offsetHeight/offsetWidth should not affect the cursor position of the currently selected input box.
Here is a live demo of the bug: http://testing.gauntlethacks.me/ffbug.html
Component: Untriaged → DOM
Product: Firefox → Core
I've gone back through previous version of Firefox to find out when this behavior started. FF 29 does not exhibit this behavior but FF 30 does.
Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d275eebfae04&tochange=b80f7eece913 My guess goes to Bug 956382. Bobby, could you comment this behavior, please.
Blocks: 956382
Flags: needinfo?(bobbyholley)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: 49 Branch → 30 Branch
This appears fixed to me in 51 (I see the broken behavior in 50). Would be curious to know what fixed it.
Flags: needinfo?(bobbyholley)
Also, thanks for the awesome bug report! Easy reduced testcases like this makes it _so_ much easier to figure out what's going on. :-)
Fix range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=505e6acd9c291504700a57ddf7e88f704f65da46&tochange=52a0d2d7639717858ce6868c19a37b95e7039736 Probably by: Decky Coss — Bug 1287655 - place textarea/input cursor at end of text when initialized; r=smaug Remaining question: is it normal that is impossible to select text (with the mouse or with shortcut like Ctrl+A) in the input field after offsetWidth/offsetHeight have been accessed?
Depends on: 1287655
I wouldn't agree that it is fixed in 51, just that the problem has moved in that the cursor gets placed at the very end instead of the very front. While this prevents the silly "typing backwards" effect it still makes it impossible to go in the middle of some text and add a space character or something.
To clarify, I tried it on the Nighlty which is 52, and saw the cursor always being forced to the back instead of the front.
Some other things to note based on further testing is that when the iframe points to something on the same exact hostname (http://domain.com) the problem doesn't show up, nor when the iframe is pointing to something completely disparate (ie, the domains don't match and the document.domains don't match). It only seems to occur when the iframe is on a different subdomain and the document.domain values are the same. Thanks for taking a look into this :)
Decky, do you have any thoughts here?
Flags: needinfo?(coss)
this reminds me of another bug that was partially solved by Bug 1287655, though it might be unrelated: Bug 1283915 it would make sense to me if it's like tslater said, that my patch did not address the root of this particular bug, only made the described behaviour less annoying. all i really did was change what happens to the selection area when a textarea or input editor is initialized. my first guess is that something in triggerBug() is repeatedly triggering the editor initialization. as far as i know, an editor can only be initialized once, so for it to occur again would mean that a new editor was created. (see https://hg.mozilla.org/integration/mozilla-inbound/file/52a0d2d76397/layout/forms/nsTextControlFrame.cpp#l231)
Flags: needinfo?(coss)
I debugged this. What happens is that we first cache the selection information in UnbindFromFrame(), then we attempt to restore them once we get a new frame. In RestoreSelectionState::Run(), we call nsTextControlFrame::SetSelectionRange() to restore the selection, however that ends up calling EnsureEditorInitialized() itself, and at the end of that function, we call SetSelectionEndPoints() <http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/layout/forms/nsTextControlFrame.cpp#314>, but there is an AutoNoJSAPI object on the stack: <http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/layout/forms/nsTextControlFrame.cpp#273>. Therefore, this call succeeds. We then return back into nsTextControlFrame::SetSelectionRange(). At this point, our selection is at the end of the text control. Then we proceed to actually restore our selection. When this code runs from line 13 of the test case, the subject principal is a codebase principal for http://testing.gauntlethacks.me/ffbug.html. The Range code then proceeds to check access to the passed node <http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/dom/base/nsRange.cpp#1190> and the check passes since the node is from the same document. However, when this code runs from line 18 of the test case (by causing the flush through accessing the offsetWidth on the iframe document's body, the subject principal is a codebase principal for http://gauntlethacks.me/ffiframe.html, so this time the security check doesn't pass, and we end up returning NS_ERROR_DOM_SECURITY_ERR. This causes the selection to stay at the end of the text control due to what EnsureEditorInitialized() does, which causes the bug. The significance of the specific setup with the iframe and using document.domain to make them same-origin is so that the test case manages to get the subject principal for the iframe right at the time where we're doing this security check to break down this house of cards. Without that, the bug shouldn't occur. Of course, the right fix is to stop doing these security check in the first place.
The Web-facing methods perform access checks which blow up when the stars are aligned such that we run this code under a subject principal that doesn't have access to the anchor node of the selection.
Assignee: nobody → ehsan
Attachment #8811505 - Flags: review?(amarchesini)
Attachment #8811505 - Flags: review?(amarchesini) → review+
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e51ba71c958 Avoid using the Web-facing Range methods in nsTextControlFrame::SetSelectionInternal(); r=baku
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Thank you for taking the time to figure out this issue and getting it resolved. I'm looking forward to FF 53 :)
(In reply to tslater2006 from comment #16) > Thank you for taking the time to figure out this issue and getting it > resolved. I'm looking forward to FF 53 :) If you need this bugfix for testing (at work e.g.), you can download Nightly to test if you want. ;) https://nightly.mozilla.org/
Just tested and confirmed! Thanks so much.
Thank _you_ for filing such a great bug report and for taking the time to minimize a test case. This was an embarrassing bug, I'm glad that it's fixed now!
Worth considering this for Aurora/Beta uplift or should we let it ride the 53 train?
Flags: needinfo?(ehsan)
Flags: in-testsuite+
Hmm, the patch here isn't very risky for an uplift, but the bug is also an edge case... I'll nominate and will let the release managers decide whether an uplift is worth it.
Flags: needinfo?(ehsan)
Comment on attachment 8811505 [details] [diff] [review] Avoid using the Web-facing Range methods in nsTextControlFrame::SetSelectionInternal() Approval Request Comment [Feature/Bug causing the regression]: This bug has probably existed in Gecko for a long time. [User impact if declined]: See comment 0. The bug is visible to web pages, but only if there is an iframe that's made same origin with the page using document.domain. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: The code change is pretty safe to uplift and is well understood (and small). [Why is the change risky/not risky?]: See the previous sentence. :-) [String changes made/needed]: None.
Attachment #8811505 - Flags: approval-mozilla-beta?
Attachment #8811505 - Flags: approval-mozilla-aurora?
Comment on attachment 8811505 [details] [diff] [review] Avoid using the Web-facing Range methods in nsTextControlFrame::SetSelectionInternal() let's get this old regression fixed in 52
Attachment #8811505 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8811505 [details] [diff] [review] Avoid using the Web-facing Range methods in nsTextControlFrame::SetSelectionInternal() Regression fix, includes tests, let's take it for 51 beta 13.
Attachment #8811505 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: