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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Don't rely on me to look at this, school starts again on Sunday.

Comment 3

[(no longer active)]

Jet, can you please find an owner?  Thanks!

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehsan any hunch on how bad this one is? (sec-?)

Comment 5

[(no longer active)]

Hard to say without further investigation.  Doesn't seem like sec-critical though.  If I have to make ratings up, how about sec-high?

Comment 6

[Daniel Veditz [:dveditz]]

bug 840098 is now fixed so we need to retest this to see if this still happens. Jesse: please retest.

Comment 7

[Jesse Ruderman]

Bug 840098 is fixed, but this still happens on trunk.

Comment 8

[Jesse Ruderman]

Attachment: 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

Comment 9

[Matt Wobensmith [:mwobensmith][:matt:]]

Jet, anyone - any update here?

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

Bug seems to be stagnating and needs some love. Can we get someone to investigate? Thanks.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Crashtests

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

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.

Comment 13

[Jesse Ruderman]

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 14

[(no longer active)]

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.

Comment 15

[Jesse Ruderman]

Do we also need to do anything for the case where nodes already in a selection are adopted into another document?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated patch

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.

Comment 17

[(no longer active)]

(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?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For checkin

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 773303 [details] [diff] [review]
For checkin

This applies as-is to aurora and beta.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b131b1f08ffc

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

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>.

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 25

[(no longer active)]

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.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Land to trunk & flag for uplift nomination?

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to [e-mail] [: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.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Any update here?  We're looking at our second-to-last beta going to build tomorrow.

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Missed the deadline for FF23 beta, will have to wait for FF24.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip for fixing mochitests

This is what I was trying to do, but I was still getting hundreds of failures.

Comment 33

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yeah, I can do it.

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Annotate tests

Comment 36

[(no longer active)]

Comment on attachment 788703 [details] [diff] [review]
Annotate tests

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

Thanks a lot, Ms2ger and Aryeh!

Comment 37

[bhavana bajaj [:bajaj]]

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 ?

Comment 38

[bhavana bajaj [:bajaj]]

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.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for checkin

This has r=ehsan and sec-a=abillings

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Second patch for checkin

Comment 41

[(no longer active)]

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.

Comment 42

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0136d838f655
https://hg.mozilla.org/integration/mozilla-inbound/rev/96cd73a17b4e

Comment 43

[bhavana bajaj [:bajaj]]

(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.

Comment 44

[(no longer active)]

The patch was backed out because of test failures. :(

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Additional mochitest fixes needed.

Note that this doesn't fix the crazy selection conformance tests.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Someone still has to debug why there were still failing conformance tests with Ms2ger's patch to the testcases.

Comment 48

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I probably didn't update the editing tests. I'll try to get to that today.

Comment 49

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Second patch for checkin (v2)

Comment 50

[(no longer active)]

Ms2ger, did you run this through try?  Is it ready to be landed?

Comment 51

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yeah, try is happy with that.

Comment 52

[(no longer active)]

Second try:

https://hg.mozilla.org/integration/mozilla-inbound/rev/38792f737cf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/94b9e5c9f994
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9cbeff3bc6

Comment 53

[Alex Keybl [:akeybl]]

(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.

Comment 54

[bhavana bajaj [:bajaj]]

Keeping comment#43 in mind, this missed the deadline for FF24 beta, will have to wait for FF25.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38792f737cf9
https://hg.mozilla.org/mozilla-central/rev/94b9e5c9f994
https://hg.mozilla.org/mozilla-central/rev/ae9cbeff3bc6

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/961150668b8a
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4c0a29943b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/d771488dca97

Comment 57

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed assertions on FF25, 2013-06-26.
Verified fixed on FF25 and FF26, 2013-10-01.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

a=bajaj on IRC for esr24 as well.

https://hg.mozilla.org/releases/mozilla-esr24/rev/18238b1051df
https://hg.mozilla.org/releases/mozilla-esr24/rev/76a0e3f839a1
https://hg.mozilla.org/releases/mozilla-esr24/rev/8acaed7d384d

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2966d5923dc4
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e6c838e2e8ee
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6e5e4fe2ee4f

https://hg.mozilla.org/releases/mozilla-esr17/rev/bb79968b80e3
https://hg.mozilla.org/releases/mozilla-esr17/rev/ac6b88bafd01
https://hg.mozilla.org/releases/mozilla-esr17/rev/54dde77e836c

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Accidentally deleted test_addRange.html.json when uplifting to b2g18/esr17. Whoops.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a20a9989255f
https://hg.mozilla.org/releases/mozilla-esr17/rev/5190dab0286e

Comment 60

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2966d5923dc4
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e6c838e2e8ee
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/6e5e4fe2ee4f
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a20a9989255f
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/4bfd6a51cd05

\m/

Comment 62

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified on 17esr and 24esr, 2013-10-15.