Land spellchecking on 1_8_BRANCH

RESOLVED FIXED in Firefox 2 alpha1

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Brett Wilson, Assigned: Brett Wilson)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 alpha1
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: See comment 4 for how to build)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
 
(Assignee)

Updated

13 years ago
Priority: -- → P1
Target Milestone: --- → Firefox 2 alpha1
(Assignee)

Updated

13 years ago
Depends on: 329672
I'm not sure you really want 253481 on the 1.8 branch.  It changed nsIPlaintextEditor.  If you just want the fix to xul:textbox it'd probably be better just to make a new patch for that.
(Assignee)

Comment 2

13 years ago
(In reply to comment #1)
> I'm not sure you really want 253481 on the 1.8 branch.  It changed
> nsIPlaintextEditor.  If you just want the fix to xul:textbox it'd probably be
> better just to make a new patch for that.

Yeah, I know. I'm going to make a new patch that fixes the constructor, but I added that bug so I wouldn't forget about it. Thanks.
(Assignee)

Updated

13 years ago
Depends on: 271498
(Assignee)

Updated

13 years ago
Blocks: 333038
No longer depends on: 333038
(Assignee)

Comment 3

12 years ago
Created attachment 218472 [details] [diff] [review]
Patch

This is the combined patch of all the bugs listed in the "depends on" list.
They are all 1_8_BRANCH approved but I had to make a few changes. The problem
was that in my branch patch on bug 302050 I added the popupEvent to
nsIDOMXULDocument2 which is a branch-only interface.

The problem is that this interface is not scriptable. So I added a new
nsIDOMXULDocument_MOZILLA_1_8_BRANCH interface. I wasn't quite sure how the
inheritance should work so I made it not inherit from anything. It seemed
strange to have nsIDOMXULDocument2 inherit from a MOZILLA_1_8_BRANCH interface,
even though they're both branch interfaces. It is only used in C++ from one
place, and it will go away as soon as the branch does so I don't think it
really matters what we do.

I added this new interface to the CLASSINFO_MAP for DOMXULDocument, and made
nsXULPopupListener QI to this interface to set and clear the popup event.
I put all of these changed files at the top of the patch. Everything starting
from firefox.js and below are unchanged from the already-approved patches.
Attachment #218472 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

12 years ago
To build you need to say "--enable-extensions=default,spellcheck" in configure.

You will also need to specify your language because there is no default (needs
to be worked out how this will be set up). You can set it temporarily for a
single box through the context menu. To permanently set it, create a configure
entry called "spellchecker.dictionary" and set it to "en-US".
Whiteboard: See comment 4 for how to build
So is it at all possible to post a diff between the reviewed stuff and this patch so I don't have to try to rereview the whole thing?

Also, how well does this code deal with content clobbering the popupEvent property?  And as far as that goes, what are the security implications of exposing even the getter of that property to content?

I really should have caught this in my initial review of the trunk checkin, but now that you mention nsIDOMXULDocument2, the security issues with sticking stuff on XULDocument are _very_ fresh in my mind.  And any patch that adds to that set worries me from a security point of view.
(Assignee)

Comment 6

12 years ago
(In reply to comment #5)
> So is it at all possible to post a diff between the reviewed stuff and this
> patch so I don't have to try to rereview the whole thing?

I put the new stuff at the beginning. Everything up to but not including firefox.js is new except for the implementations of Get/SetPopupEvent. It's very little code.
 
> Also, how well does this code deal with content clobbering the popupEvent
> property?  And as far as that goes, what are the security implications of
> exposing even the getter of that property to content?
>
> I really should have caught this in my initial review of the trunk checkin, but
> now that you mention nsIDOMXULDocument2, the security issues with sticking
> stuff on XULDocument are _very_ fresh in my mind.  And any patch that adds to
> that set worries me from a security point of view.

I have absolutely no idea. Perhaps dveditz can comment. I didn't realize this was accessable to web pages, which does sound scary.

All we really need is a getter for rangeParent and rangeOffset from the event. Perhaps we can expose these by themselves as getters only to script. The document already exports popupNode which seems like it would have the same security implications.
Note that bug 319315 is still very much an issue for me with trunk builds
Note that it's not the chrome's popupEvent stuff that would be accessible to content.  But if I load a XUL file off the web, it gets a XULDocument, and that XULDocument will have this property hanging off it, and for popup event in that document content can do what it pleases.

popupNode is somewhat protected by our same-origin policies (at least as far as the getter goes), since nodes know what their principal is.  I'm not sure what having popupNode be settable means security-wise either.

Basically, what things actually NEED to be able to get and set popupEvent?  I'd say the safe thing to do is to limit it as much as possible given those constraints.  If it's only set from C++, the setter should be noscript.  If it's only gotten from chrome JS, the getter should do a corresponding security check.    That sort of thing.
(Assignee)

Updated

12 years ago
Blocks: 335291
(Assignee)

Updated

12 years ago
Attachment #218472 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

12 years ago
Created attachment 219675 [details] [diff] [review]
Patch addressing security concerns

This patch contains the files that have changed since the trunk patches. Anything not here is the same as the previous patch except for a few trivial JS argument changes to get the new events.

I moved the popupEvent to the trusted DOMXULDocument2. I put readonly attributes for the node and the offset on the scriptable nsIDOMXULDocument_MOZILLA_1_8_BRANCH. These call through to the popup event getter.
Attachment #219675 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #218472 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Blocks: 335306
Comment on attachment 219675 [details] [diff] [review]
Patch addressing security concerns

r=bzbarsky.  Looks great; thanks for doing this!

We should make sure to get something like this on trunk too, right?
Attachment #219675 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

12 years ago
(In reply to comment #10)
> (From update of attachment 219675 [details] [diff] [review] [edit])
> r=bzbarsky.  Looks great; thanks for doing this!
> 
> We should make sure to get something like this on trunk too, right?

I already filed bug 335291 for this.
(Assignee)

Comment 12

12 years ago
Created attachment 219900 [details] [diff] [review]
Patch to check in
Attachment #219675 - Attachment is obsolete: true
(Assignee)

Comment 13

12 years ago
On 1.8 branch. See bug 335306 for turning on by default.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.