Selection::addRange should reject ranges that has a different root

RESOLVED FIXED in Firefox -esr45

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Depends on 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 fixed, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(4 attachments)

http://w3c.github.io/selection-api/#dom-selection-addrange says:
"
The method must set the context object's range to range by a strong reference (not by making a copy) if the root ([DOM4]) of the range's boundary points are the document associated with context object. Otherwise, this method must do nothing.
"

It seems we don't implement the "Otherwise, this method must do nothing" bit.
Here's a patch that does that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83f7f845a013e69b04fc605ce3b42ee9e2e619cb
Oops, that broke Selection objects that are associated with text form controls.
Here's a new try that fixes that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82609a005ef6ca0eb154f9427887439e54d6ea65
(FTR, GetRoot() returns the <input> element in that case.)
A little refactoring to support giving a document in a param instead
of Selection::GetParentObject() (which needs a mFrameSelection).
Attachment #8839654 - Flags: review?(bugs)
FWIW, I tested Chrome briefly and we're now compatible for this case AFAICT.
Comment on attachment 8839652 [details] [diff] [review]
part 1 - Make Selection::addRange silently reject ranges that have a different root.

># HG changeset patch
># User Mats Palmgren <mats@mozilla.com>
># Parent  4e41289d88c7cf5fd539083ead2bf415ecd7a249
>Bug 1341137 part 1 - Make Selection::addRange silently reject ranges that have a different root.  r=smaug
>
>diff --git a/layout/generic/nsSelection.cpp b/layout/generic/nsSelection.cpp
>--- a/layout/generic/nsSelection.cpp
>+++ b/layout/generic/nsSelection.cpp
>@@ -4959,16 +4959,26 @@ Selection::AddRange(nsIDOMRange* aDOMRan
>   ErrorResult result;
>   AddRange(*range, result);
>   return result.StealNSResult();
> }
> 
> void
> Selection::AddRange(nsRange& aRange, ErrorResult& aRv)
> {
>+  auto rangeRoot = aRange.GetRoot();
>+  auto doc = GetParentObject();
Since it is non-obvious what kind of objects GetRoot and GetParentObject return, please use concrete type, not 'auto'.
We have had leaks because of similar-ish case hiding the actual type. And use of 'auto' has lead to security critical bugs too, though
can't now recall in which context.
Attachment #8839652 - Flags: review?(bugs) → review+
(ok, part 2 isn't trivial to review, will continue tomorrow.)
Comment on attachment 8839654 [details] [diff] [review]
part 2 - Provide a way to add ranges to Selection objects that aren't associated with a shell/nsFrameSelection.

I'm having hard time to prove that in
nsCopySupport::GetTransferableForNode aDoc == aNode->OwnerDoc()
But I guess if that isn't the case, things don't work anyhow. 

Same applies to nsHTMLCopyEncoder::SetSelection
Attachment #8839654 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #7)
> >+  auto rangeRoot = aRange.GetRoot();
> >+  auto doc = GetParentObject();
> Since it is non-obvious what kind of objects GetRoot and GetParentObject
> return, please use concrete type, not 'auto'.

It's obviously returning some nsINode-ish thing, in some pointer or
smart-pointer form.  None of those possible types leaks by using 'auto'
as far as I know.

> We have had leaks because of similar-ish case hiding the actual type. And
> use of 'auto' has lead to security critical bugs too, though
> can't now recall in which context.

I'd be interested to know exactly how that happened, if you can dig it up.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f32de880c77
part 1 - Make Selection::addRange silently reject ranges that have a different root.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac44e1e713e1
part 2 - Provide a way to add ranges to Selection objects that aren't associated with a shell/nsFrameSelection.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/343d11a33afe
part 3 - Update a reftest reference to account for the new AddRange behavior.
TLDR; the test failures are expected, I just forgot to update the WPT manifests

This patch is the result of "./mach web-platform-tests-update wpt_raw.log"
from the failed run.

The new failures falls into three categories:
1. adding a range with "detached" (i.e. an element created by the same document
as the range but not inserted into any document) boundary point
2. adding a range with "foreign" (i.e. an element created by a different
document from the range and inserted in that other document) boundary point
3. adding a range with "detachedForeign" (i.e. an element created by a different
document from the range but not inserted into any document) boundary point

Such ranges are now intentionally rejected.  I hacked the addRange-00.html
file so I could load it in the URL bar and then compared the results with
Chrome.  It fails in the same way on the tests that failed here, which
indicates we're getting more compatible (and possibly the tests themselves
could now be updated with the new expected behavior).

I'll re-land once the Try run becomes green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e64cceb89b4e55e437f8ec60773ede8131caf3e
Flags: needinfo?(mats)
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9764fc002e8
part 1 - Make Selection::addRange silently reject ranges that have a different root.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f4db063fe8
part 2 - Provide a way to add ranges to Selection objects that aren't associated with a shell/nsFrameSelection.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd74df6d77f
part 3 - Update a reftest reference to account for the new AddRange behavior.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb61b6e1e6b3
part 4 - Update web-platform-tests expected data.
Marking this in-testsuite+ since it's covered by WPT.
Flags: in-testsuite+
Comment on attachment 8839652 [details] [diff] [review]
part 1 - Make Selection::addRange silently reject ranges that have a different root.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: possible webcompat issues since Chrome implements this
[Is this code covered by automated tests?]: yes, WPT
[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]:
[Is the change risky?]:no
[Why is the change risky/not risky?]:the change is fairly small and straightforward
[String changes made/needed]:none
Attachment #8839652 - Flags: approval-mozilla-esr52?
Attachment #8839652 - Flags: approval-mozilla-beta?
Attachment #8839652 - Flags: approval-mozilla-aurora?
(The request is for all four patches)
Comment on attachment 8839652 [details] [diff] [review]
part 1 - Make Selection::addRange silently reject ranges that have a different root.

make addRange behave according to spec, aurora53+/beta52+ (and release52+, if uplift happens after today's merge).  This is for all 4 patches in this bug.
esr52 should be synced from beta or release, so clearing separate approval flag.
Attachment #8839652 - Flags: approval-mozilla-esr52?
Attachment #8839652 - Flags: approval-mozilla-beta?
Attachment #8839652 - Flags: approval-mozilla-beta+
Attachment #8839652 - Flags: approval-mozilla-aurora?
Attachment #8839652 - Flags: approval-mozilla-aurora+
So why didn't you update the tests?
Flags: needinfo?(mats)
Depends on: 1342907
Filed bug 1342907 on updating the WPT tests.
Flags: needinfo?(mats)
Those failures looks unlikely to have been caused by these patches.
Are you sure it's not one of the other changes in the same push?
Bug 1337826 for example?
Flags: needinfo?(mats) → needinfo?(cbook)
Ah, now I see there are a few addRange.html failures in there too...
Hmm, looking...
Flags: needinfo?(cbook)
At first glance, these looks like the same failures that we had without
part 4.  So, I'm guessing we need to regenerate the manifest using 'mach'
on the branch, because the format is different or something...
I'll try that...
Yeah, "mach web-platform-tests-update" resulted in a much different patch
on Aurora so I relanded it with that.
Comment on attachment 8839652 [details] [diff] [review]
part 1 - Make Selection::addRange silently reject ranges that have a different root.

This would be good to get on ESR45 as well. See comment 17 for justification.
Attachment #8839652 - Flags: approval-mozilla-esr45?
Comment on attachment 8839652 [details] [diff] [review]
part 1 - Make Selection::addRange silently reject ranges that have a different root.

Per discussion with Ryan, we need this in ESR45. ESR45+.
Attachment #8839652 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-esr45/rev/e86e0423dad1
https://hg.mozilla.org/releases/mozilla-esr45/rev/9aebee8b8cb9
https://hg.mozilla.org/releases/mozilla-esr45/rev/69f3d44bdb48
https://hg.mozilla.org/releases/mozilla-esr45/rev/22546e2cee64

The wpt results suggest that more manifest updates are needed, but I'm having a difficult time getting that command to work locally. I'll follow up with jgraham tomorrow.
You need to log in before you can comment on or make changes to this bug.