627 bytes, text/html
39.57 KB, text/plain
713 bytes, text/html
2.99 KB, patch
Away for a while: review+
|Details | Diff | Splinter Review|
2.73 KB, patch
|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|
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.
Don't rely on me to look at this, school starts again on Sunday.
Jet, can you please find an owner? Thanks!
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.
Bug 840098 is fixed, but this still happens on trunk.
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.
Created attachment 772160 [details] [diff] [review] Crashtests
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.
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.
Do we also need to do anything for the case where nodes already in a selection are adopted into another document?
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.
(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 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 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.
Created attachment 773303 [details] [diff] [review] For checkin
Comment on attachment 773303 [details] [diff] [review] For checkin This applies as-is to aurora and beta.
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 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.
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?
(In reply to email@example.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.
Any update here? We're looking at our second-to-last beta going to build tomorrow.
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.
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.
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.
Comment on attachment 788703 [details] [diff] [review] Annotate tests Review of attachment 788703 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot, Ms2ger and Aryeh!
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.
Created attachment 796975 [details] [diff] [review] Patch for checkin This has r=ehsan and sec-a=abillings
Created attachment 796976 [details] [diff] [review] Second patch for checkin
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.
(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. :(
Created attachment 797529 [details] [diff] [review] Additional mochitest fixes needed. Note that this doesn't fix the crazy selection conformance tests.
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
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.
Created attachment 798890 [details] [diff] [review] Second patch for checkin (v2)
Ms2ger, did you run this through try? Is it ready to be landed?
Yeah, try is happy with that.
(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.
Keeping comment#43 in mind, this missed the deadline for FF24 beta, will have to wait for FF25.
https://hg.mozilla.org/mozilla-central/rev/38792f737cf9 https://hg.mozilla.org/mozilla-central/rev/94b9e5c9f994 https://hg.mozilla.org/mozilla-central/rev/ae9cbeff3bc6
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
Confirmed assertions on FF25, 2013-06-26. Verified fixed on FF25 and FF26, 2013-10-01.
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
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/
Verified on 17esr and 24esr, 2013-10-15.