Closed Bug 860123 Opened 11 years ago Closed 11 years ago

"ASSERTION: Must have the same owner document" with editor, sel.selectAllChildren()

Categories

(Core :: DOM: Editor, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox22 --- wontfix
firefox23 + wontfix
firefox24 + wontfix
firefox25 + verified
firefox26 --- verified
firefox-esr17 25+ verified
firefox-esr24 25+ verified
b2g18 25+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: jruderman, Assigned: mrbkap)

References

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [adv-main25+][adv-esr1710+][adv-esr24-1+] )

Attachments

(7 files, 6 obsolete files)

627 bytes, text/html
Details
39.57 KB, text/plain
Details
713 bytes, text/html
Details
2.99 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
2.73 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.31 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
768.73 KB, patch
Details | Diff | Splinter Review
Attached file testcase
###!!! ASSERTION: Must have the same owner document: 'HasSameOwnerDoc(NODE_FROM(aParent, aDocument))', file content/base/src/Element.cpp, line 1028

###!!! ASSERTION: aDocument must be current doc of aParent: '!aParent || aDocument == aParent->GetCurrentDoc()', file content/base/src/Element.cpp, line 1030

###!!! ASSERTION: Unexpected aDocument: 'aDocument == mDocument', file layout/base/nsPresShell.cpp, line 4025

###!!! ASSERTION: must be in the same rule tree as parent: 'r1 == r2', file layout/style/nsStyleContext.cpp, line 67

Security-sensitive in case this is related to bug 840098.
Attached file stacks
Don't rely on me to look at this, school starts again on Sunday.
Jet, can you please find an owner?  Thanks!
Assignee: nobody → bugs
Ehsan any hunch on how bad this one is? (sec-?)
Hard to say without further investigation.  Doesn't seem like sec-critical though.  If I have to make ratings up, how about sec-high?
bug 840098 is now fixed so we need to retest this to see if this still happens. Jesse: please retest.
Flags: needinfo?(jruderman)
Keywords: sec-high
Bug 840098 is fixed, but this still happens on trunk.
Flags: needinfo?(jruderman)
Attached file testcase 2
This one goes further off the rails.

###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1494

or 

###!!! ASSERTION: Must have data if restyle bit is set: 'gotData', file ../../../layout/base/RestyleTracker.cpp, line 271

Assertion failure: inFlowFrame (How did that happen?), at ../../../layout/base/nsCSSFrameConstructor.cpp:9109
Jet, anyone - any update here?
Bug seems to be stagnating and needs some love. Can we get someone to investigate? Thanks.
Flags: needinfo?(bugs)
Attached patch CrashtestsSplinter Review
Assignee: bugs → mrbkap
Attachment #772160 - Flags: review?(ehsan)
Attached patch Proposed fix (obsolete) — Splinter Review
Aryeh, this might need to make its way into the spec.

The bug here is that we end up in editor code dealing with nodes that aren't ours. In general, it doesn't make sense for a document's selection to be in another document, so this patch prevents that from happening.
Attachment #772163 - Flags: review?(ehsan)
Attachment #772163 - Flags: feedback?(ayg)
Flags: needinfo?(bugs)
Attachment #772160 - Flags: review?(ehsan) → review+
Comment on attachment 772163 [details] [diff] [review]
Proposed fix

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

>    // Delete all of the current ranges

Move this comment down?
Comment on attachment 772163 [details] [diff] [review]
Proposed fix

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

I can't find where Extend calls Collapse...  I think we need this check in both places.
Attachment #772163 - Flags: review?(ehsan)
Do we also need to do anything for the case where nodes already in a selection are adopted into another document?
Attached patch Updated patch (obsolete) — Splinter Review
I could have sworn that ranges threw if you tried to make them span documents. However, instead, that just makes them collapse to the new node.
Attachment #772163 - Attachment is obsolete: true
Attachment #772163 - Flags: feedback?(ayg)
Attachment #772227 - Flags: review?(ehsan)
Attachment #772227 - Flags: feedback?
(In reply to Jesse Ruderman from comment #15)
> Do we also need to do anything for the case where nodes already in a
> selection are adopted into another document?

I believe that should work correctly, since as part of adopting a node we remove it from its parent in the original document.  Do you have a test case which shows this is a problem?
Attachment #772227 - Flags: review?(ehsan)
Attachment #772227 - Flags: review+
Attachment #772227 - Flags: feedback?(ayg)
Attachment #772227 - Flags: feedback?
Comment on attachment 772227 [details] [diff] [review]
Updated patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not very easily. The patch points to selections and nodes from a different document, but the bug here involves fixed position elements and contenteditable (or maybe also just a button) which is a pretty big leap to make.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't think so.

Which older supported branches are affected by this flaw?

All of them.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They should not be too risky and the backport is not difficult.

How likely is this patch to cause regressions; how much testing does it need?

This is unlikely to cause regressions, it's a very targeted patch that only affects the case that could case problems.
Attachment #772227 - Flags: sec-approval?
Comment on attachment 772227 [details] [diff] [review]
Updated patch

sec-approval+ for trunk. We'll want this on the beta, aurora, and ESR branches so please prepare patches for those too.
Attachment #772227 - Flags: sec-approval? → sec-approval+
Attached patch For checkin (obsolete) — Splinter Review
Attachment #773303 - Flags: sec-approval+
Attachment #773303 - Flags: review+
Keywords: checkin-needed
Comment on attachment 773303 [details] [diff] [review]
For checkin

This applies as-is to aurora and beta.
Attachment #773303 - Flags: approval-mozilla-beta?
Attachment #773303 - Flags: approval-mozilla-aurora?
Comment on attachment 773303 [details] [diff] [review]
For checkin

We can't do this: the spec apparently requires that we are allowed to put the selection in a foreign document (see http://hg.mozilla.org/mozilla-central/annotate/dde4dcd6fa46/dom/imptests/editing/selecttest/common.js#l43 for an example). The patch turned the tree orange and was backed out in <https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb8af704e0d>.
Attachment #773303 - Attachment is obsolete: true
Attachment #773303 - Flags: sec-approval+
Attachment #773303 - Flags: review+
Attachment #773303 - Flags: approval-mozilla-beta?
Attachment #773303 - Flags: approval-mozilla-aurora?
Comment on attachment 772227 [details] [diff] [review]
Updated patch

Don't consider the editing spec authoritative at all.  It's a very rough thing and isn't actively maintained last I checked.  If the test complains, it's okay to add a new expected fail for the test if necessary, assuming we do actually want the changed behavior.

I don't have time to look at patches right now, so I'm canceling the feedback request.
Attachment #772227 - Flags: feedback?(ayg)
OK, so it looks like IE10 does what we currently do, and WebKit and Blink do not allow selections to leak into other documents but do that without throwing.  I think not throwing doesn't make sense, but WebKit/Blink's behavior doesn't make sense either.  So, I think we should take the current patch, fix up the test expectation to make it pass, and land it, and be prepared to deal with possible regressions.
Land to trunk & flag for uplift nomination?
Flags: needinfo?(mrbkap)
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #26)
> Land to trunk & flag for uplift nomination?

This tried to land and bounced (see comment 23). I'm working on fixing the test failures now.
Flags: needinfo?(mrbkap)
Any update here?  We're looking at our second-to-last beta going to build tomorrow.
Flags: needinfo?(mrbkap)
Missed the deadline for FF23 beta, will have to wait for FF24.
I tried to make the tests pass, but still have a bunch of failures to figure out. I'll try again next week. fwiw, rushing this into beta was a pretty scary proposition.
Flags: needinfo?(mrbkap)
I have less and less time to look into this. If anybody else can help with the mochitests, that would probably lead to this bug getting fixed the fastest.
Attached patch wip for fixing mochitests (obsolete) — Splinter Review
This is what I was trying to do, but I was still getting hundreds of failures.
The test files are conformance tests for the spec and shouldn't be changed out of sync with the spec -- that defeats the point of conformance tests!  Instead, we should mark the failures as expected, which (correctly) signals that we have deviated from the spec here.  If the spec should change, a spec bug should be filed separately.

The procedure for updating the list of expected failures is here, under the heading "parseFailures.py":

https://hg.mozilla.org/mozilla-central/file/3d20597e0a07/dom/imptests/README

Basically you modify the file dom/imptests/testharnessreport.js so dumpFailures is true; do ./mach mochitest-plain dom/imptests/editing/selecttest > /tmp/log to run the tests and save the output; and then cd dom/imptests; python parseFailures.py < /tmp/log.  This should automatically update dom/imptests/failures/editing/selecttest/test_addRange.html.json to reflect the new expected failures.  Review the diff to make sure it looks reasonable and you didn't cause failures elsewhere etc., set dumpFailures back to false, and you're done.

That's in theory.  It's possible it won't work quite that nicely in practice, in which case I suggest asking Ms2ger or me for help.  If you have trouble, I can try to handle this myself, but I have extremely limited time.  Ms2ger might be willing to do it too; CCing him.
Yeah, I can do it.
Attached patch Annotate tests (obsolete) — Splinter Review
Comment on attachment 788703 [details] [diff] [review]
Annotate tests

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

Thanks a lot, Ms2ger and Aryeh!
Attachment #788703 - Flags: review+
Blake,what are the next steps here to help resolve this in Fx24 timeframe ? Comment #36 looks like the tests are reviewed, is this landing away ?
Just got in touch with Blake and he should be landing this shortly in time to get it in on our next Beta for tomorrow morning.
Keywords: checkin-needed
Attachment #787824 - Attachment is obsolete: true
This has r=ehsan and sec-a=abillings
Attachment #772227 - Attachment is obsolete: true
Attachment #796975 - Flags: sec-approval+
Attachment #796975 - Flags: review+
Attached patch Second patch for checkin (obsolete) — Splinter Review
Attachment #788703 - Attachment is obsolete: true
Attachment #796976 - Flags: review+
Comment on attachment 796975 [details] [diff] [review]
Patch for checkin

(Filling out the approval information once instead of three times!)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not clear.
User impact if declined: Security sensitive.
Testing completed (on m-c, etc.): Locally.
Risk to taking this patch (and alternatives if risky): There is some risk that this might break content which is relying on this broken behavior.
String or IDL/UUID changes made by this patch: None.
Attachment #796975 - Flags: approval-mozilla-esr17?
Attachment #796975 - Flags: approval-mozilla-beta?
Attachment #796975 - Flags: approval-mozilla-b2g18?
Attachment #796975 - Flags: approval-mozilla-aurora?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #41)
> Comment on attachment 796975 [details] [diff] [review]
> Patch for checkin
> 
> (Filling out the approval information once instead of three times!)
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Not clear.
> User impact if declined: Security sensitive.
> Testing completed (on m-c, etc.): Locally.
> Risk to taking this patch (and alternatives if risky): There is some risk
> that this might break content which is relying on this broken behavior.
> String or IDL/UUID changes made by this patch: None.

Given this has not had a minimum bake time on m-c, I cannot approve this for today's beta.After which, we are in our final two week cycle where we really recommend only low risk fixes.

Based on the risk evaluation here its unclear if the fallout's would be easily discoverable and fixed by the time Fx24 hits release.So I will be calling this a wontfix on Fx24 and approve for aurora.Please comment here if there is any disagreement to that.
The patch was backed out because of test failures. :(
Note that this doesn't fix the crazy selection conformance tests.
Attachment #797529 - Flags: review?(ehsan)
Attachment #797529 - Flags: review?(ehsan) → review+
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
Attachment #796975 - Flags: approval-mozilla-b2g18?
Someone still has to debug why there were still failing conformance tests with Ms2ger's patch to the testcases.
I probably didn't update the editing tests. I'll try to get to that today.
Flags: needinfo?(Ms2ger)
Attachment #796976 - Attachment is obsolete: true
Flags: needinfo?(Ms2ger)
Ms2ger, did you run this through try?  Is it ready to be landed?
Flags: needinfo?(Ms2ger)
Yeah, try is happy with that.
Flags: needinfo?(Ms2ger)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> b2g18 is frozen for non-leo+ blockers. Please request blocking status if you
> feel that this must land there.

I'm sorry, I fucked this up. I fed RyanVM bad information. The v1.1 branch will be open until 3/3/2014, as it's being maintained for security until that time.
Attachment #796975 - Flags: approval-mozilla-b2g18?
Keeping comment#43 in mind, this missed the deadline for FF24 beta, will have to wait for FF25.
Attachment #796975 - Flags: approval-mozilla-beta?
Attachment #796975 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Confirmed assertions on FF25, 2013-06-26.
Verified fixed on FF25 and FF26, 2013-10-01.
Status: RESOLVED → VERIFIED
Status: VERIFIED → RESOLVED
Closed: 11 years ago11 years ago
Attachment #796975 - Flags: approval-mozilla-esr17?
Attachment #796975 - Flags: approval-mozilla-esr17+
Attachment #796975 - Flags: approval-mozilla-b2g18?
Attachment #796975 - Flags: approval-mozilla-b2g18+
And because I was being completely brain-dead when I rebased this, yet another bustage fix.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4bfd6a51cd05
https://hg.mozilla.org/releases/mozilla-esr17/rev/bdc3de3699e3

On the bright side, it's building and passing tests now!
Whiteboard: [adv-main25+]
Whiteboard: [adv-main25+] → [adv-main25+][adv-esr1710+][adv-esr24-1+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.