479093 - Text sent to services includes body of <script> tags

Status: RESOLVED FIXED

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

Description

[Ed Lee :Mardak]

I was happily using the speech service to read a news article and all of a sudden it started talking about "Math dot floor nine nine nine nine nine... if document dot write..."

Apparently there was a script tag for an advertisement block in the middle of the article. (Adblock blocked it, but the script was inline.)

In particular I was reading http://www.news-gazette.com/news/transportation/2009/01/13/zipcars_making_debut_in_cu but I'm sure any inline script tag will show this bug.

Comment 1

[Tom Dyas]

The services code uses the NS_QUERY_SELECTED_TEXT event to obtain the text of the current selection.  The code that generates the text is in content/events/src/nsContentEventHandler.cpp.  That file contains its own code to loop over the DOM elements and copy out plain text.  It does not appear to skip over SCRIPT elements.

This is in contrast to the clipboard code, which uses the nsIDocumentEncoder interface and plain text serializers over in content/base/src, which can be configured to skip SCRIPT elements (among other things).

The solution maybe is to modify the code in nsContentEventHandler.cpp to use nsIDocumentEncoder and/or the related support code in some fashion.  This would also have the effect of removing duplicate DOM traversal code in that file.

Comment 2

[Ed Lee :Mardak]

(In reply to comment #1)
> The solution maybe is to modify the code in nsContentEventHandler.cpp to use
> nsIDocumentEncoder and/or the related support code in some fashion.  This would
> also have the effect of removing duplicate DOM traversal code in that file.
Would there be a reason why it has this potential duplicate DOM traversal code?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This speech service thing is a TSF feature right? I mean, it requires TSF to be enabled?

We'd have to think carefully about this. NS_QUERY_SELECTED_TEXT is used to send text to IMEs for processing, it's pretty important that normal DOM text is sent back exactly as entered. I don't know enough about the serializer to know whether it's suitable for that.

Skipping display:none elements would fix the <script> issue and maybe be a good idea in general. It would probably break people using IMEs to edit display:none text, but I guess that's not a real issue.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually it probably wouldn't break people editing display:none text, as long as the query started inside the display:none element, since the children of a display:none element are not normally display:none.

Comment 5

[Tom Dyas]

A good solution might be to extend nsContentEventHandler.cpp to support a "NS_QUERY_SELECTED_TEXT_AS_TRANSFERABLE."  The implementation would use the clipboard code in content/base/* to generate a nsITransferable from the current selection as it would for cut/paste.  This nsITransferable can then be posted to the services pasteboard using the existing code in widget/src/cocoa.  The benefit is that it should solve the SCRIPT tag issue but also allow the posting of HTML content in addition to the plain text so other services can see the formatting.

Comment 6

[Tom Dyas]

And it would leave NS_QUERY_SELECTED_TEXT alone completely.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It seems that it's enough for roc's approach (nsContentEventHandler don't use the text nodes which is invisible (display: none; only??)).

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> It seems that it's enough for roc's approach

I meant: roc's approach is enough for this bug.

And also I think that nsContentEventHandler should not depend on other modules. Because the modules changes will be very risky for TSF and Mac's similar APIs if the changes are not for TSF bugs.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, the Mac services code ... Maybe that shouldn't even be using QUERY_SELECTED_TEXT. Maybe it should use the serializer directly.

Comment 10

[Tom Dyas]

My thought exactly.  I'm going to work up a patch along the lines in my comment #5 and leverage as much of the clipboard helpers as I can.

Comment 11

[Tom Dyas]

Attachment: use clipboard support code for services v1

This patch changes the services code to use the clipboard support code to create a nsITransferable and then post that transferable to the services pasteboard.  The services pasteboard should no longer contains script tag data.

If this patch solves the problem, I'll need to delete the commented-out code and refactor nsCopySupport::HTMLCopy and the new nsCopySupport::GetTransferable.

Comment 12

[Tom Dyas]

Attachment: use clipboard support code for services v2

This patch adds two new NS_QUERY_CONTENT_EVENTs : NS_QUERY_HAS_SELECTION and NS_QUERY_SELECTION_AS_TRANSFERABLE.  I refactored nsCopySupport::HTMLCopy so there is no code duplication between nsCopySupport::HTMLCopy and the new nsCopySupport::GetTransferableForSelection.  Cut/copy still works after the refactoring.

Ed: Does this patch fix the issue for you?

Comment 13

[Ed Lee :Mardak]

(In reply to comment #12)
> Ed: Does this patch fix the issue for you?
Yup. I applied it locally and it's not speaking the javascript code anymore.

Comment 14

[Tom Dyas]

Attachment: use clipboard support code for services v2.1

Cleaned up the patch to not change white space unnecessarily.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 364022 [details] [diff] [review]
use clipboard support code for services v2.1

>+static nsresult
>+SelectionCopyHelper(nsISelection *aSel, nsIDocument *aDoc,
>+                    PRBool doPutOnClipboard, PRInt16 aClipboardID,
>+                    nsITransferable ** aTransferablePtr)
> {
I'd name the last parameter aTransferable.
And please set *aTransferable to null in the beginning of the method.
if (aTransferable) {
  *aTransferable = nsnull; 
}

>+nsresult
>+nsCopySupport::GetTransferableForSelection(nsISelection * aSel,
>+                                           nsIDocument * aDoc,
>+                                           nsITransferable ** aTransferablePtr)
aTransferable

> nsresult
>+nsContentEventHandler::OnQueryHasSelection(nsQueryContentEvent * aEvent)
>+{
>+  nsresult rv = Init(aEvent);
>+  if (NS_FAILED(rv))
>+    return rv;
>+  
>+  PRBool isCollapsed;
>+  rv = mSelection->GetIsCollapsed(&isCollapsed);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  aEvent->mSucceeded = ! isCollapsed;
Extra space after '!'


>@@ -1634,6 +1634,18 @@
>       handler.OnQueryEditorRect((nsQueryContentEvent*)aEvent);
>     }
>     break;
>+  case NS_QUERY_HAS_SELECTION:
>+    {
>+      nsContentEventHandler handler(mPresContext);
>+      handler.OnQueryHasSelection((nsQueryContentEvent*)aEvent);
>+    }
>+    break;
>+  case NS_QUERY_SELECTION_AS_TRANSFERABLE:
>+    {
>+      nsContentEventHandler handler(mPresContext);
>+      handler.OnQuerySelectionAsTransferable((nsQueryContentEvent*)aEvent);
In the new code, please use C++ casting: static_cast<nsQueryContentEvent*>(aEvent)

I'm not familiar enough with the OSX related code.

Comment 16

[Tom Dyas]

Attachment: use clipboard support code for services v2.2

Modified as per Olli Pettay's comments.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +// Query whether a selection currently exists. mReply.mSucceeded will contain
> +// the result (PR_FALSE or PR_TRUE).
> +#define NS_QUERY_HAS_SELECTION           (NS_QUERY_CONTENT_EVENT_START + 6)

I think we don't need this event. I guess you worry the performance, but roc thought it's not important at this event. You should keep current implementation.

> +nsContentEventHandler::OnQueryHasSelection(nsQueryContentEvent * aEvent)
> +{
> +  nsresult rv = Init(aEvent);
> +  if (NS_FAILED(rv))
> +    return rv;
> +  
> +  PRBool isCollapsed;
> +  rv = mSelection->GetIsCollapsed(&isCollapsed);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  aEvent->mSucceeded = !isCollapsed;

and also I don't like this. mSucceeded should not be used for the result. It should be used only for the succeeded or failed state.

I think even if you don't like the current behavior (using NS_QUERY_SELECTED_TEXT), you can check whether the selection is collapsed or not in nsContentEventHandler::OnQuerySelectionAsTransferable.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Ah, right, NS_QUERY_HAS_SELECTION isn't perhaps needed.
Just use NS_QUERY_SELECTION_AS_TRANSFERABLE where you now use NS_QUERY_HAS_SELECTION and check if there is transferable available.
Shouldn't that work reasonable well?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Though, might be good to test that the performance is ok if a huge page
(like http://www.whatwg.org/specs/web-apps/current-work/ ) is selected.
If NS_QUERY_HAS_SELECTION gives us significantly better performance, then
adding it does make sense.

Comment 20

[Tom Dyas]

I intentionally designed it that way because, when the Services menu is opened, Mac OS X calls validRequestorForSendType:returnType: once for each item in the menu multiplied by the number of possible data types each service can support.  Then writeSelectionToPasteboard:types: is called only if the user actually selects a menu item.

I added some timing and counting code this morning to get specific numbers.  validRequestorForSendType:returnType: was called 140 times just after the menu was opened and each call took less than 1 microsecond.  (The amount of time was less than the resultion of PRIntervalTime.)

writeSelectionToPasteboard:types: was called once upon my selection of a service and took 540,000 microseconds.  I had selected all of the text on slashdot.org's main page.

Looping 140 times through the DOM and throwing away the result is something that should be avoided ...

Comment 21

[Tom Dyas]

Unfortunately, the transferable cannot be cached since Mac OS X does not inform you when the Services menu has opened for the first time only that it needs to know whether a transfer with specific data types is supported at the time of the method call.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 364451 [details] [diff] [review]
use clipboard support code for services v2.2

Thanks for testing!
I don't see any problem using event.mSucceeded as it is used in the patch.
I know Masayuki disagrees with me.

Comment 23

[Tom Dyas]

I can add a mHasSelection to the mReply structure in nsQueryContentEvent.  I see Masayuki's point that mSucceeded is used more for the success/fail state of the query event.  I was just trying to avoid adding another member to mReply so it doesn't get littered with variables only used for a single message.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #23)
> I can add a mHasSelection to the mReply structure in nsQueryContentEvent.  I
> see Masayuki's point that mSucceeded is used more for the success/fail state of
> the query event.  I was just trying to avoid adding another member to mReply so
> it doesn't get littered with variables only used for a single message.

Adding mHasSelection is better for me. Because the clients of the event may need to know whether the event handling is succeeded or failed. And it can be set in nsContentEventHandler::Init at all events. Then, we can rename NS_QUERY_HAS_SELECTION to a better name, so it can be used for widgets to get several common information. E.g., you can use the new event for |conversationIdentifier| of nsChildView that is just getting mRootContent.

Note that I intentionally positioned mSucceeded to outside of mReply. I hope that the meaning of mSucceeded will not be broken.

Comment 25

[Tom Dyas]

(In reply to comment #24)
> 
> Adding mHasSelection is better for me. Because the clients of the event may
> need to know whether the event handling is succeeded or failed. And it can be
> set in nsContentEventHandler::Init at all events. Then, we can rename
> NS_QUERY_HAS_SELECTION to a better name, so it can be used for widgets to get
> several common information. E.g., you can use the new event for
> |conversationIdentifier| of nsChildView that is just getting mRootContent.

How about NS_QUERY_CONTENT_STATE as the name?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #25)
> How about NS_QUERY_CONTENT_STATE as the name?

Sounds good for me.

Comment 27

[Tom Dyas]

Attachment: use clipboard support code for services v3

Changed NS_QUERY_HAS_SELECTION to NS_QUERY_CONTENT_STATE.  Updated code to use mSucceeded only for event success/fail state.  Updated widget code to inform Mac OS X about HTML support.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 364789 [details] [diff] [review]
use clipboard support code for services v3


>+  if ((!sendType
>+       || [sendType isEqual:NSStringPboardType]
>+       || [sendType isEqual:NSHTMLPboardType])
>+      && !returnType) {
Nit, usually it looks better, IMO, if || and && are in the end of a line, not
starting a new line.

Comment 29

[Tom Dyas]

IMHO, when they are at the front of the line, your eye is naturally drawn to them, which makes it easier to understand the conditional expression.  When I'm reading other people's code, I always hate having to look to the end of the line to know what the connector is.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree, but putting them at the end-of-line is our standard coding style.

Comment 31

[Tom Dyas]

Attachment: use clipboard support code for services v3.1

Put conditional operators at end of line.  Consolidated the conditional expression into two lines.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 365231 [details] [diff] [review]
use clipboard support code for services v3.1

I'm not going to continue nitpicking ;)

Comment 33

[Tom Dyas]

I don't mind the nits.  Keeps the code quality high.  But if I had been the one to write the coding style guidelines, connectors would be required to be at the front of each line! :)

Comment 34

[Josh Aas]

Comment on attachment 365231 [details] [diff] [review]
use clipboard support code for services v3.1

>+  if ([types containsObject:NSStringPboardType] == NO
>+      && [types containsObject:NSHTMLPboardType] == NO)

Please put the "&&" at the end of the first line, not at the beginning of the second. This happens in a couple of places.

Comment 35

[Tom Dyas]

Attachment: use clipboard support code for services v3.2

Fix remaining conditionals.

Comment 36

[Ed Lee :Mardak]

Comment on attachment 367930 [details] [diff] [review]
use clipboard support code for services v3.2

Smaug and Josh already gave you the r+. No need to re-request r? for minor changes. :)

Comment 37

[Ed Lee :Mardak]

Attachment: hg exported patch for checkin

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/ca4d8bb54e7f