861607 - ASSERTION: Map should be empty already: 'clearedEntries == 0' running bidi reftests

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

With the patch from bug 861606, running reftests in layout/reftests/bidi/dirAuto asserts a lot:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/smontagu/mozwork/hgtree/mozilla/layout/reftests/bidi/dirAuto/dynamicDirAuto-setRTL-Auto6.html | assertion count 20 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/smontagu/mozwork/hgtree/mozilla/layout/reftests/bidi/dirAuto/dynamicDirAuto-setRTL-InvalidDir1.html | assertion count 15 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/smontagu/mozwork/hgtree/mozilla/layout/reftests/bidi/dirAuto/dynamicDirAuto-ChangeText-RTL4.html | assertion count 17 is more than expected 0 assertions

All the assertions are:
 ASSERTION: Map should be empty already: 'clearedEntries == 0', file /home/smontagu/mozwork/hgtree/mozilla/content/base/src/DirectionalityUtils.cpp, line 551

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

These calls to ClearHasDirAutoSet are not right: if we clear the flag here in AfterSetAttr, we then don't go through the right code path in WalkAncestorsResetAutoDirection (called from OnSetDirAttr), and don't clear the old entries from TextNodeDirectionalityMap.

Comment 2

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f62f6a7e09

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38f62f6a7e09

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 737217 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 548206 or one of its followups
User impact if declined: without bug 861606 this could lead to sec-crit vulnerabilities (see dependencies for examples). With bug 861606 its impact is mitigated, but since risk is very low I'd like to get this in for the first release of dir=auto
Testing completed (on m-c, etc.): baked on m-c since 2013-04-15
Risk to taking this patch (and alternatives if risky): minimal
String or IDL/UUID changes made by this patch: none

Comment 5

[bhavana bajaj [:bajaj]]

Comment on attachment 737217 [details] [diff] [review]
Patch

Approving the low risk uplift to avoid the dirauto sec-crit vulnerabilities .Please make sure to land on the beta branch by Monday Apr 22 for this to make it into Fx 21 beta 4

Comment 6

[Simon Montagu :smontagu]

https://hg.mozilla.org/releases/mozilla-aurora/rev/762431b69db5
https://hg.mozilla.org/releases/mozilla-beta/rev/c9948f38753a

Comment 7

[u279076]

Are there any user facing regression risks QA should be testing not covered by your tests?

Comment 8

[Simon Montagu :smontagu]

I think the existing tests cover it.

Comment 9

[u279076]

Thanks Simon, marking [qa-].