Closed Bug 998188 Opened 10 years ago Closed 10 years ago

ContentEventHandler::OnQueryCaretRect() converts native text offset with wrong node

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(7 files, 7 obsolete files)

34.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.98 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.95 KB, patch
jimm
: review+
Details | Diff | Splinter Review
2.90 KB, patch
Margaret
: review+
capella
: feedback+
Details | Diff | Splinter Review
6.25 KB, patch
Details | Diff | Splinter Review
22.46 KB, patch
Details | Diff | Splinter Review
8.48 KB, patch
Details | Diff | Splinter Review
This bug causes crash on debug build in TSF mode.

>     uint32_t offset;
>     rv = GetFlatTextOffsetOfRange(mRootContent, mFirstSelectedRange, &offset);
>     NS_ENSURE_SUCCESS(rv, rv);
>     // strip out native new lines, we want the non-native offset. The offsets
>     // handed in here are from selection, caretPositionFromPoint, and editable
>     // element offset properties. We need to match those or things break. 
>     nsINode* startNode = mFirstSelectedRange->GetStartParent();
>     if (startNode && startNode->IsNodeOfType(nsINode::eTEXT)) {
>       offset = ConvertToXPOffset(static_cast<nsIContent*>(startNode), offset);
>     }

For example, on twitter.com, the editor for tweet is created by <div contenteditable>. Then, GetFlatTextOffsetOfRange() computes the offset with children of the <div>. However, ConvertToXPOffset() only refers currently editing text node. If there is composition string, the startNode is dummy text node for the composition string. I.e., it doesn't contain the text before the startNode.

Additionally, WidgetQueryContentEvent::mInput::mOffset must be native offset. I don't understand why Jimm and Ehsan changed this in bug 858526, though.

I believe that we should backout the patch for bug 858526 and if it's needed, bug 858526 should be fixed with another approach.
Perhaps, I'll separate part.1 for easier to review.
Automated test for reproducing this crash bug.
Attachment #8409634 - Attachment is obsolete: true
Attachment #8409635 - Attachment is obsolete: true
Attachment #8409636 - Attachment is obsolete: true
Attachment #8410153 - Flags: review?(ehsan)
The cause of this regression is, JS component may want to use nsIDOMWindowUtils.sendQueryContentEvent() and nsIDOMWindowUtils.sendSelectionSetEvent() with XP line break since native line break isn't useful for JS.

This patch makes ContentEventHandler can work with both native line break and XP line break. If mUseNativeLineBreak of WidgetQueryContentEvent or WidgetSelectionEvent is true, the offset and length are treated as in string including native line breaks. And the result string is generated with native line breaks. Otherwise, the offset and length are treated as in string including XP line breaks (\n). And the result string is generated with XP line breaks.

For easy to read the callers, some methods of ContentEventHandler takes LineBreakType instead of boolean.
Attachment #8410161 - Flags: review?(bugs)
This patch sorts out variable names, offset and length in ContentEventHandler.

If they are ambitious, i.e., it may be native offset/length and XP offset/length which are switched by LineBreakType argument, the name should be just offset/length.

If they are XP offset/length, the names should be xpOffset/xpLength.

If they are native offset/length, the names should be nativeOffset/nativeLength.

If they are node offset/length, the names should be nodeOffset/nodeLength.
Attachment #8410165 - Flags: review?(bugs)
This patch adds aAdditionalFlags to nsIDOMWindowUtils.sendQueryContentEvent() and nsIDOMWindowUtils.sendSelectionSetEvent().

This change does NOT break compatibility with old style. The compatibility is tested in test_sendQueryContentAndSelectionSetEvent.html.
Attachment #8410166 - Flags: review?(bugs)
Fixes the tests added by bug 858526 with additional flags. The tests want nsIDOMWindowUtils.sendQueryContentEvent() work with XP offset.

And also improves EventUtils.js and ChromeUtils.js by explicitly specifying additional flags.
Attachment #8410168 - Flags: review?(bugs)
Comment on attachment 8410153 [details] [diff] [review]
part.1 Add test for this crash on debug build

Review of attachment 8410153 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +1000,5 @@
> +function synthesizeQueryCaretRect(aOffset, aWindow)
> +{
> +  var utils = _getDOMWindowUtils(aWindow);
> +  if (!utils) {
> +    return nullptr;

Not sure who put this in, but s/ptr//.

::: widget/tests/test_bug998188.html
@@ +8,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body onload="runTests();">

Instead of this, please use waitForFocus().
Attachment #8410153 - Flags: review?(ehsan) → review+
Thank you, Ehsan.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #12)
> ::: widget/tests/test_bug998188.html
> @@ +8,5 @@
> > +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> > +  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
> > +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> > +</head>
> > +<body onload="runTests();">
> 
> Instead of this, please use waitForFocus().

Yeah, I did it. However, on Mac OS X 10.6, it causes permanent orange.

See https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=c1231c979bb8

> 23:57:05     INFO -  2326 INFO TEST-START | /tests/uriloader/exthandler/tests/mochitest/test_badMimeType.html
> 23:57:05     INFO -  2327 INFO TEST-INFO | MEMORY STAT vsize after test: 3973943296
> 23:57:05     INFO -  2328 INFO TEST-INFO | MEMORY STAT residentFast after test: 450830336
> 23:57:05     INFO -  2329 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 95835128
> 23:57:05     INFO -  2330 INFO TEST-END | /tests/uriloader/exthandler/tests/mochitest/test_badMimeType.html | finished in 95ms
> 23:57:05     INFO -  2331 INFO TEST-START | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml
> 23:57:06     INFO -  2332 INFO TEST-INFO | MEMORY STAT vsize after test: 3976806400
> 23:57:06     INFO -  2333 INFO TEST-INFO | MEMORY STAT residentFast after test: 449814528
> 23:57:06     INFO -  2334 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 106153360
> 23:57:06     INFO -  2335 INFO TEST-END | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml | finished in 755ms
> 23:57:09     INFO -  2336 INFO Error: Unable to restore focus, expect failures and timeouts.
> 23:57:09     INFO -  2337 INFO TEST-START | /tests/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml
> 23:57:12     INFO -  2338 INFO Error: Unable to restore focus, expect failures and timeouts.
> 23:57:13     INFO -  2339 INFO TEST-INFO | MEMORY STAT vsize after test: 3968774144
> 23:57:13     INFO -  2340 INFO TEST-INFO | MEMORY STAT residentFast after test: 439832576
> 23:57:13     INFO -  2341 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 92097848
> 23:57:13     INFO -  2342 INFO TEST-END | /tests/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml | finished in 4331ms
> 23:57:16     INFO -  2343 INFO Error: Unable to restore focus, expect failures and timeouts.
> 23:57:16     INFO -  2344 INFO TEST-START | /tests/widget/tests/test_bug998188.html
> 23:57:19     INFO -  2345 INFO Error: Unable to restore focus, expect failures and timeouts.
> 00:02:30     INFO -  2346 INFO TEST-INFO | dumping last 2 message(s)
> 00:02:30     INFO -  2347 INFO TEST-INFO | if you need more context, please use SimpleTest.requestCompleteLog() in your test
> 00:02:30     INFO -  2348 INFO TEST-INFO | /tests/widget/tests/test_bug998188.html | must wait for load
> 00:02:30     INFO -  2349 INFO TEST-INFO | /tests/widget/tests/test_bug998188.html | must wait for focus
> 00:02:31     INFO -  TEST-INFO | screencapture: exit 0
> 00:02:31     INFO -  2350 INFO TEST-UNEXPECTED-FAIL | /tests/widget/tests/test_bug998188.html | Test timed out.
> 00:02:31     INFO -  2351 INFO TEST-UNEXPECTED-FAIL | /tests/widget/tests/test_bug998188.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0

After test_handlerApps.xhtml, looks like our window never becomes active. I think that this is bug 933303.

So, I think, we have two choice.

1. Use onload event.
2. Disables the test on Mac.

The test doesn't need native focus. Therefore, I chose #1. How do you think about this?
Flags: needinfo?(ehsan)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> After test_handlerApps.xhtml, looks like our window never becomes active. I
> think that this is bug 933303.

Yes indeed.  Looking at the screenshot, that's what it is.

> So, I think, we have two choice.
> 
> 1. Use onload event.
> 2. Disables the test on Mac.
> 
> The test doesn't need native focus. Therefore, I chose #1. How do you think
> about this?

Well, we have a third option, move the test to somewhere before test_handlerApps.xhtml to make sure that we're not affected by this problem right?  Please feel free to move it to somewhere in editor/.
Flags: needinfo?(ehsan)
Okay, I'll move it, thanks again.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7)
> Created attachment 8410165 [details] [diff] [review]
> part.3 Sort out offset/length variable names in ContentEventHandler
> 
> If they are ambitious, i.e., it may be native offset/length and XP
> offset/length which are switched by LineBreakType argument, the name should
> be just offset/length.

Ugh, I meant "If they are ambiguous, ..."
Comment on attachment 8410161 [details] [diff] [review]
part.2 ContentEventHandler should support 2 modes, native line break mode and XP line break mode

// true if use native line breaks for mOffset and mLength
true if native line breaks are used for mOffset and mLength
Attachment #8410161 - Flags: review?(bugs) → review+
Comment on attachment 8410165 [details] [diff] [review]
part.3 Sort out offset/length variable names in ContentEventHandler

I guess this is fine. offset naming is still a bit odd, but we have so
many different offsets.
Attachment #8410165 - Flags: review?(bugs) → review+
Comment on attachment 8410166 [details] [diff] [review]
part.4 nsIDOMWindowUtils.sendQueryContentEvent() and .sendSelectionSetEvent() should take additional flags with its argument for making callers selectable native line break mode or XP line break mode


>+  is(caretXP.top, caretNative.top,
>+     "The caret top should be same (got: XP, expected: Native)");
I don't understand this types of messages. Why the got-expected part?
I would just drop that here and elsewhere


>   /**
>    * Synthesize a query content event. Note that the result value returned here
>    * is in LayoutDevice pixels rather than CSS pixels.
>    *
>    * @param aType  One of the following const values.  And see also each comment
>    *               for the other parameters and the result.
>+   * @param aAdditionalFlags See following QUERY_CONTENT_FLAG_* flags.
Could you define the flags before using them

>    */
>+  /**
>+   * If SELECTION_SET_FLAG_REVERSE is set, the selection set from
>+   * |aOffset + aLength| to |aOffset|.  Otherwise, set from |aOffset| to
>+   * |aOffset + aLength|.
>+   */
>+  const unsigned long SELECTION_SET_FLAG_REVERSE               = 0x0001;
>+  /**
>+   * If SELECTION_SET_FLAG_USE_XP_LINE_BREAK is set to aAdditionalFlags,
>+   * plain text generated from content is created with "\n".  Otherwise,
>+   * platform dependent.  E.g., on Windows, "\r\n" is used.
>+   * aOffset and aLength are offset and length in/of the plain text content.
>+   */
>+  const unsigned long SELECTION_SET_FLAG_USE_NATIVE_LINE_BREAK = 0x0000;
>+  const unsigned long SELECTION_SET_FLAG_USE_XP_LINE_BREAK     = 0x0010;
I don't understand which flag ordering,  I'd start from 0000 and go from there.
Attachment #8410166 - Flags: review?(bugs) → review+
Comment on attachment 8410168 [details] [diff] [review]
part.5 Fix orange and improve EventUtils.js and ChromeUtils.js

update the flags in EventUtils.js to follow nsIDOMWindowUtils flags.
Attachment #8410168 - Flags: review?(bugs) → review+
About SELECTION_SET_FLAG_* definition in nsIDOMWindowUtils.idl, do you expect this style?

I.e., the order is,

  const unsigned long SELECTION_SET_FLAG_USE_NATIVE_LINE_BREAK = 0x0000;
  const unsigned long SELECTION_SET_FLAG_REVERSE               = 0x0001;
  const unsigned long SELECTION_SET_FLAG_USE_XP_LINE_BREAK     = 0x0010;

Note that SELECTION_SET_FLAG_REVERSE needs to be 1 since true is 1. This keeps the old style code behavior.
Attachment #8410166 - Attachment is obsolete: true
Attachment #8411464 - Flags: feedback?(bugs)
Comment on attachment 8410169 [details] [diff] [review]
part.6 MetroFirefox should use XP line break at calling sendQueryContentEvent() in its chrome

I think that this is what you expected and wanted change in bug 858526.
Attachment #8410169 - Flags: review?(jmathies)
Comment on attachment 8410170 [details] [diff] [review]
part.7 Firefox for Android should use XP line break at calling sendQueryContentEvent() in its chrome

These callers must expect the offset/length and the string of its result are generated with "\n". Although, "\n" is used on Android when QUERY_CONTENT_FLAG_USE_NATIVE_LINE_BREAK, let's use right argument in its logic.
Attachment #8410170 - Flags: review?(margaret.leibovic)
 
> Note that SELECTION_SET_FLAG_REVERSE needs to be 1 since true is 1. This
> keeps the old style code behavior.
Oh, that is error prone. Can we really not just fix all the callers to use flags, and not bool params.
(In reply to Olli Pettay [:smaug] from comment #25)
>  
> > Note that SELECTION_SET_FLAG_REVERSE needs to be 1 since true is 1. This
> > keeps the old style code behavior.
> Oh, that is error prone. Can we really not just fix all the callers to use
> flags, and not bool params.

I fixed all callers in our tree by the patches. However, it's possible to be used by add-ons...
https://mxr.mozilla.org/addons/ doesn't seem to reveal such addons.
Attachment #8410169 - Flags: review?(jmathies) → review+
(In reply to Olli Pettay [:smaug] from comment #27)
> https://mxr.mozilla.org/addons/ doesn't seem to reveal such addons.

Oh, I've not known that. Hmm, then, XP_LINE_BREAK should be 0x0001. It's same value as for sendQueryContentEvent().
Comment on attachment 8410170 [details] [diff] [review]
part.7 Firefox for Android should use XP line break at calling sendQueryContentEvent() in its chrome

Review of attachment 8410170 [details] [diff] [review]:
-----------------------------------------------------------------

I also want capella to take a look at this.
Attachment #8410170 - Flags: feedback?(markcapella)
Comment on attachment 8410170 [details] [diff] [review]
part.7 Firefox for Android should use XP line break at calling sendQueryContentEvent() in its chrome

Review of attachment 8410170 [details] [diff] [review]:
-----------------------------------------------------------------

wfm - I wouldn't mind a brief comment before the consumers along the lines of "Chrome observes XP styled line-breaks", but I'm overly obsessive and it's not a deal breaker.
Attachment #8410170 - Flags: feedback?(markcapella) → feedback+
jst: Could you sr the interface change? nsIDOMWindowUtils.sendSelectionSet() breaks compatibility with current build. However, we confirmed that this method isn't used in any addons which can be checked in mxr.
Attachment #8411464 - Attachment is obsolete: true
Attachment #8411464 - Flags: feedback?(bugs)
Attachment #8412436 - Flags: superreview?(jst)
Comment on attachment 8412436 [details] [diff] [review]
part.4 nsIDOMWindowUtils.sendQueryContentEvent() and .sendSelectionSetEvent() should take additional flags with its argument for making callers selectable native line break mode or XP line break mode

Looks good, just a couple of comment tweak suggestions below:

   /**
+   * If QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK is set to aAdditionalFlags of
+   * sendQueryContentEvent(), plain text generated from content is created with

Maybe word that something like:

If sendQueryContentEvent()'s aAdditionalFlags argument is QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK plain text generated from content is created with...

   /**
+   * If QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK is set to aAdditionalFlags of
+   * sendSelectionSetEvent(), aOffset and aLength are offset and length in/of

Same here (language wise), and replace QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK with SELECTION_SET_FLAG_USE_XP_LINE_BREAK.

   const unsigned long QUERY_CHARACTER_AT_POINT                  = 3208;
 
+
   /**

Remove the addition of that empty line.

+  /**
+   * If SELECTION_SET_FLAG_REVERSE is set, the selection set from

Add "is" between selection and is.

+   * |aOffset + aLength| to |aOffset|.  Otherwise, set from |aOffset| to

Add "it is" before "set".

+   * @param aAdditionalFlags See following SELECTION_SET_FLAG_*.

Maybe say "See the description of SELECTION_SET_FLAG_*."
Attachment #8412436 - Flags: superreview?(jst) → superreview+
Attachment #8410170 - Flags: review?(margaret.leibovic) → review+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.