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

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

({assertion, sec-high, testcase})

Trunk
mozilla26
x86_64
Mac OS X
assertion, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox22 wontfix, firefox23+ wontfix, firefox24+ wontfix, firefox25+ verified, firefox26 verified, firefox-esr1725+ verified, firefox-esr2425+ verified, b2g1825+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [adv-main25+][adv-esr1710+][adv-esr24-1+] )

Attachments

(7 attachments, 6 obsolete attachments)

627 bytes, text/html
Details
39.57 KB, text/plain
Details
713 bytes, text/html
Details
2.99 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
2.73 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.31 KB, patch
Away for a while
: review+
Details | Diff | Splinter Review
768.73 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 735515 [details]
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.
(Reporter)

Comment 1

5 years ago
Created attachment 735516 [details]
stacks
Don't rely on me to look at this, school starts again on Sunday.

Comment 3

5 years ago
Jet, can you please find an owner?  Thanks!
Assignee: nobody → bugs
Ehsan any hunch on how bad this one is? (sec-?)

Comment 5

5 years ago
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
(Reporter)

Comment 7

5 years ago
Bug 840098 is fixed, but this still happens on trunk.
Flags: needinfo?(jruderman)
(Reporter)

Comment 8

5 years ago
Created attachment 745729 [details]
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)
(Assignee)

Comment 11

4 years ago
Created attachment 772160 [details] [diff] [review]
Crashtests
Assignee: bugs → mrbkap
Attachment #772160 - Flags: review?(ehsan)
(Assignee)

Comment 12

4 years ago
Created attachment 772163 [details] [diff] [review]
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.
Attachment #772163 - Flags: review?(ehsan)
Attachment #772163 - Flags: feedback?(ayg)
Flags: needinfo?(bugs)

Updated

4 years ago
Attachment #772160 - Flags: review?(ehsan) → review+
(Reporter)

Comment 13

4 years ago
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

4 years ago
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)
(Reporter)

Comment 15

4 years ago
Do we also need to do anything for the case where nodes already in a selection are adopted into another document?
(Assignee)

Comment 16

4 years ago
Created attachment 772227 [details] [diff] [review]
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.
Attachment #772163 - Attachment is obsolete: true
Attachment #772163 - Flags: feedback?(ayg)
Attachment #772227 - Flags: review?(ehsan)
Attachment #772227 - Flags: feedback?

Comment 17

4 years ago
(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?

Updated

4 years ago
Attachment #772227 - Flags: review?(ehsan)
Attachment #772227 - Flags: review+
Attachment #772227 - Flags: feedback?(ayg)
Attachment #772227 - Flags: feedback?
(Assignee)

Comment 18

4 years ago
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?
status-b2g18: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox23: --- → +
tracking-firefox24: --- → +
tracking-firefox25: --- → +
tracking-firefox-esr17: --- → 23+
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+
(Assignee)

Comment 20

4 years ago
Created attachment 773303 [details] [diff] [review]
For checkin
Attachment #773303 - Flags: sec-approval+
Attachment #773303 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 21

4 years ago
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?
https://hg.mozilla.org/integration/mozilla-inbound/rev/b131b1f08ffc
Flags: in-testsuite?
Keywords: checkin-needed
(Assignee)

Comment 23

4 years ago
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)

Comment 25

4 years ago
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)
(Assignee)

Comment 27

4 years ago
(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)

Updated

4 years ago
tracking-firefox-esr17: 23+ → ?
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.
status-firefox23: affected → wontfix
(Assignee)

Comment 30

4 years ago
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)
(Assignee)

Comment 31

4 years ago
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.
(Assignee)

Comment 32

4 years ago
Created attachment 787824 [details] [diff] [review]
wip for fixing mochitests

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.
Created attachment 788703 [details] [diff] [review]
Annotate tests

Comment 36

4 years ago
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.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #787824 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
Created attachment 796975 [details] [diff] [review]
Patch for checkin

This has r=ehsan and sec-a=abillings
Attachment #772227 - Attachment is obsolete: true
Attachment #796975 - Flags: sec-approval+
Attachment #796975 - Flags: review+
(Assignee)

Comment 40

4 years ago
Created attachment 796976 [details] [diff] [review]
Second patch for checkin
Attachment #788703 - Attachment is obsolete: true
Attachment #796976 - Flags: review+

Comment 41

4 years ago
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?

Comment 42

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0136d838f655
https://hg.mozilla.org/integration/mozilla-inbound/rev/96cd73a17b4e
Keywords: checkin-needed
(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

4 years ago
The patch was backed out because of test failures. :(
(Assignee)

Comment 45

4 years ago
Created attachment 797529 [details] [diff] [review]
Additional mochitest fixes needed.

Note that this doesn't fix the crazy selection conformance tests.
Attachment #797529 - Flags: review?(ehsan)

Updated

4 years ago
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.
status-b2g18: affected → wontfix
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → wontfix
status-firefox26: --- → affected
Attachment #796975 - Flags: approval-mozilla-b2g18?
(Assignee)

Comment 47

4 years ago
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)
Created attachment 798890 [details] [diff] [review]
Second patch for checkin (v2)
Attachment #796976 - Attachment is obsolete: true
Flags: needinfo?(Ms2ger)

Comment 50

4 years ago
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)

Comment 52

4 years ago
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
(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.
status-b2g18: wontfix → affected

Updated

4 years ago
Attachment #796975 - Flags: approval-mozilla-b2g18?

Updated

4 years ago
status-b2g-v1.1hd: wontfix → affected
Keeping comment#43 in mind, this missed the deadline for FF24 beta, will have to wait for FF25.
status-firefox24: affected → wontfix

Updated

4 years ago
Attachment #796975 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/38792f737cf9
https://hg.mozilla.org/mozilla-central/rev/94b9e5c9f994
https://hg.mozilla.org/mozilla-central/rev/ae9cbeff3bc6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox26: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

4 years ago
tracking-firefox-esr17: ? → 25+

Updated

4 years ago
Attachment #796975 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox25: affected → fixed
tracking-b2g18: ? → 25+
Depends on: 917515
Confirmed assertions on FF25, 2013-06-26.
Verified fixed on FF25 and FF26, 2013-10-01.
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
status-firefox26: fixed → verified
status-firefox-esr24: --- → affected
tracking-firefox-esr24: --- → 25+
Status: VERIFIED → RESOLVED
Last Resolved: 4 years ago4 years ago

Updated

4 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+
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
status-b2g18: affected → fixed
status-b2g-v1.2: --- → fixed
status-firefox-esr17: affected → fixed
status-firefox-esr24: affected → fixed
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
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!
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/
status-b2g-v1.1hd: affected → fixed
Whiteboard: [adv-main25+]
Whiteboard: [adv-main25+] → [adv-main25+][adv-esr1710+][adv-esr24-1+]
Verified on 17esr and 24esr, 2013-10-15.
status-firefox-esr17: fixed → verified
status-firefox-esr24: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.