Nonfatal assertion or scary crash, depending on how long the testcase was open: 1. Load 2. Quit ###!!! ASSERTION: element already removed from map: 'mElements.Contains(aElement)', file dom/base/DirectionalityUtils.cpp, line 469 1. Load 2. Wait for GC 3. Quit heap-buffer-overflow #2 mozilla::nsTextNodeDirectionalityMap::RemoveElementFromMap #3 mozilla::ResetDir ... #12 nsCycleCollector::ShutdownCollect

rax = 0xe5e5e5e5e5e5e5e5, but crash address is 0x0000000d

> rax = 0xe5e5e5e5e5e5e5e5, but crash address is 0x0000000d So it was mostly luck that I found this bug despite having bug 836925 in my ignore lists

To Mats for a look...

FWIW, I tried the patch in bug 836925 but that didn't help. It might be the same issue as bug 900997?

The issue here is very subtle. When we're setting the dir to "auto" on the span, we get to this code: <https://dxr.mozilla.org/mozilla-central/source/dom/base/DirectionalityUtils.cpp#509> under this stack: #0 mozilla::nsTextNodeDirectionalityMap::ResetNodeDirection (aEntry=0x7f110adf3140, aData=0x7f110adea4c0) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:519 #1 0x00007f11271a56ba in nsCheapSet<nsPtrHashKey<mozilla::dom::Element> >::EnumerateEntries (this=0x7f110adf3140, aEnumFunc=0x7f112719da79 <mozilla::nsTextNodeDirectionalityMap::ResetNodeDirection(nsPtrHashKey<mozilla::dom::Element>*, void*)>, aUserArg=0x7f110adea4c0) at /home/ehsan/moz/src/xpcom/ds/nsCheapSets.h:79 #2 0x00007f112719dc33 in mozilla::nsTextNodeDirectionalityMap::ResetAutoDirection (this=0x7f110adf3140, aTextNode=0x7f110adea4c0) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:538 #3 0x00007f112719de77 in mozilla::nsTextNodeDirectionalityMap::ResetTextNodeDirection (aTextNode=0x7f110adea4c0) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:577 #4 0x00007f112716d0ed in mozilla::WalkDescendantsResetAutoDirection (aElement=0x7f110adea430) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:684 #5 0x00007f112716d8f9 in mozilla::OnSetDirAttr (aElement=0x7f110adea430, aNewValue=0x7ffde504a840, hadValidDir=false, hadDirAuto=false, aNotify=true) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:923 #6 0x00007f11271769ac in mozilla::dom::Element::SetAttrAndNotify (this=0x7f110adea430, aNamespaceID=0, aName=0x7f1114df09d0, aPrefix=0x0, aOldValue=..., aParsedValue=..., aModType=2 '\002', aFireMutation=false, aNotify=true, aCallAfterSetAttr=true) at /home/ehsan/moz/src/dom/base/Element.cpp:2420 Here, rootNode is the <bdi> element which doesn't have a dir attribute yet. But HasDirAuto() returns true for it, since it's a <bdi> element! <https://dxr.mozilla.org/mozilla-central/source/dom/base/Element.h#353> Therefore, we start going into the weeds, like this. We call WalkDescendantsSetDirectionFromText(), and it first finds the "f" text node, but it won't use it because that's aChangedNode, so it keeps looking, and then it finds the "g" text node and incorrectly determines that "g" determines the directionality of our <bdi> node. Later when we set the dir="auto" attribute on the <bdi> element itself, we correctly decide that "f" determines the directionality of the <bdi> and later when we remove the span, we correctly clear the directionality map entry for "f" but not "g" (since that is in an inconsistent state) and we lose. IOW, this bug can only be triggered by this *exact* test case. ;-) The fix is very simple. We need to run the downward propagation algorithm only if we've encountered a real dir=auto element, not a <bdi> without one.

Comment on attachment 8704388 [details] [diff] [review] Only run the downward propagation algorithm for elements with a real dir=auto attribute I'm not really familiar with this stuff. Like, I have no idea when "downward propagation algorithm" should run and when not. Based on Bug 613149, peterv might be better reviewer for this code. Feels _very _error prone to have a method called HasDirAuto() which doesn't mean what we want here. The code has 1491 // Set if the node has dir=auto. 1492 NodeHasDirAuto That clearly hints that attribute value is auto. But based on the code <bdi> sets the flag too. So the comment is wrong at least, but I'd say also the flag name and its setter/getter. Did you go through other HasDirAuto() usage? So, I would change HasDirAuto() implementation to bool HasDirAuto() { return HasFixedDir() && HasValidDir(); } and then rename NodeHasDirAuto to something else. NodeBehavesAsDirAuto or some such. But ok, that becomes rather large change, not anything for branches.

Comment on attachment 8704388 [details] [diff] [review] Only run the downward propagation algorithm for elements with a real dir=auto attribute There are some test failures on try...

Ehsan is this something we should be trying to get onto 44? Is it affected? Are you still working on the patch? Or is the problem in the patch? Thanks. We should track this for 44+ since it's sec-critical, but it looks like it may end up wontfix for 44.

I will gtb Beta9 tomm and enter RC mode next week. If a stable fix is ready and nominated for uplift soon, we can consider taking the fix in Beta44.

I'm still struggling to come up with a good fix, but it's been years since I looked at this stuff. Also I still think that my analysis on the edge casey-ness of this is correct, so I'm not in a rush to get this fixed in 44.

I'll just mark this wontfix in 44 for now. Thanks for investigating this issue, Ehsan.

Tracking for 47.

This is likely heading for a wontfix for 45 and 46 as well. Are we ok with shipping several releases in a row with this sec-critical issue, even if it is an edge case?

I'll try to prioritize this... (Note that I'm traveling next week and there is a good chance I won't get to this one this week.)

After further investigation, I think most of comment 7 is correct, but the conclusion I originally drew from it is not. The real problem here is that WalkDescendantsSetDirectionFromText() thinks that the "f" text node is being changed, which is the beginning of things getting into an inconsistent state. We use this function from three places: the call site in question, TextNodeChangedDirection() and ResetDirectionSetByTextNode(). In the latter two cases, the contents of the text node are changing in a way that make it impossible for it to affect the direction of dir=auto ancestors. However, in the call site in question, all we're doing is re-resolving the auto direction of any possible ancestors, but we know that the text node hasn't necessarily changed in a way to make it no longer responsible for any dir=auto ancestors. Therefore, in this call site we should be passing null for aChangedNode. Once that happens, following comment 7, the "f" text node will be picked, which is the correct text node to choose here.

I had a question about oldTextNode at https://dxr.mozilla.org/mozilla-central/source/dom/base/DirectionalityUtils.cpp#506, which is then passed to WalkDescendantsSetDirectionFromText as aChangedNode. We cast it to Element*, but from your comments here it sounds like it's really a text node. WalkDescendantsSetDirectionFromText has this check: |child->NodeType() == nsIDOMNode::TEXT_NODE && child != aChangedNode|, which if aChangedNode is an element is an odd condition. Do you know what's really going on there?

This argument is only useful when it points to a text node (hence the check in WalkDescendantsSetDirectionFromText). The cast to Element there is wrong, and we're getting lucky since the pointer math turns out to the correct case. Also calling this "text node" along the way is not really helpful. I submitted a patch to improve things in bug 1257208 to keep the cleanup separate from this bug.

Comment on attachment 8728062 [details] [diff] [review] Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor [Security approval request comment] How easily could an exploit be constructed based on the patch? Not very easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. The commit message basically describes the code change in English. Which older supported branches are affected by this flaw? All. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, and very easy (if the patch doesn't apply verbatim.) How likely is this patch to cause regressions; how much testing does it need? It passed our existing tests. Still I think some baking time would be needed since the code in question is pretty complex.

Comment on attachment 8728062 [details] [diff] [review] Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor sec-approval+. We'll want Aurora and Beta patches made and nominated. I suspect we'll want this on ESR38 and ESR45 also.

Comment on attachment 8728062 [details] [diff] [review] Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor Please refer to comment 22, I won't repeat the same info here.

Ehsan, you suggested some bake time in comment 22. Is a week in Nightly enough for this patch before uplifting to all affected branches? More?

(In reply to Ritu Kothari (:ritu) from comment #26) > Ehsan, you suggested some bake time in comment 22. Is a week in Nightly > enough for this patch before uplifting to all affected branches? More? Honestly I don't know how to judge how much time is the right amount, so sure. I haven't seen any regressions from this yet.

Landing now in beta (for beta 5) means we have some time to fix possible regressions in beta. So I am inclined to uplift now.

Comment on attachment 8728062 [details] [diff] [review] Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor Crash/assertion fix, let's land this for beta 5.

Comment on attachment 8728062 [details] [diff] [review] Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor sec critical, taking it Should be in 45.1.0 & 38.8.0

This doesn't apply cleanly to esr38.

This is the ESR38 patch. Unfortunately with this patch, the following tests fail: 1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-dir-L.html | image comparison (==), max difference: 255, number of differing pixels: 4505 1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-dir-R.html | image comparison (==), max difference: 255, number of differing pixels: 4473 1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-invalid-dir-L.html | image comparison (==), max difference: 255, number of differing pixels: 4505 1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-invalid-dir-R.html | image comparison (==), max difference: 255, number of differing pixels: 4473 Unfortunately I don't have cycles to look into why any time soon. Mats, can you please help here? Much appreciated!

I have no clue how this feature is supposed to work, sorry.

Sylvestre, how long is ESR38 going to be supported for? I'm really short on free time these days and would like to avoid spending time on the ESR38 port of this patch if at all possible.

Ehsan, Next week's ESR38 (ESR38.8) is the last release. Technically, given its rating, we'd normally fix this in ESR38 given our normal criteria.

I am not happy about doing it but I think we should just wontfix it... Let's see if we are lucky...

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/de5ff2a8024f Add a crashtest based on the test case for the bug