This is part of Bug 922963.

This is a work-in-progress patch. It builds, but I need to do testing and add unit tests.

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c6651b676e

Added ref test.

Fixing compiler warning on Windows.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40fd98cb4711

Comment on attachment 8598432 [details] [diff] [review] bug-1157726-isolates.patch Review of attachment 8598432 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the nits below fixed. Thank you again for taking this on! ::: layout/base/nsBidi.cpp @@ +445,5 @@ > * Get the directional properties for the text, > * calculate the flags bit-set, and > * determine the partagraph level if necessary. > */ > +nsresult nsBidi::GetDirProps(const char16_t *aText) No need to make this not void if always returns NS_OK @@ +464,5 @@ > + LOOKING_FOR_PDI /* 3: found strong after FSI, looking for PDI */ > + }; > + State state; > + > + /* The following stacks are used to manage isolate sequences. Thos Nit: "Those" @@ +466,5 @@ > + State state; > + > + /* The following stacks are used to manage isolate sequences. Thos > + sequences may be nested, but obviously never more deeply than the > + maxium explicit embedding level. Nit: "maximum" @@ +479,5 @@ > + int32_t stackLast = -1; > + > + if(isDefaultLevel) { > + /* > + * see comment in nsIBidi.h: "nsBidi.h" @@ +558,5 @@ > + } > + break; > + > + case B: > + // This shouldn't happen, since we don't support multiple paragraphs. Maybe add an NS_NOTREACHED here? @@ +576,3 @@ > } > + > + /* Resolve direciton of still unresolved open FSI sequences */ Nit: "direction" @@ +629,5 @@ > * > * > * Handling the stack of explicit levels (Xn): > * > + * With the Bidi stack of explicit levels, as pushed with each Nit: trailing whitespace @@ +640,5 @@ > * > * This implementation assumes that NSBIDI_MAX_EXPLICIT_LEVEL is odd. > */ > > +nsresult nsBidi::ResolveExplicitLevels(nsBidiDirection *aDirection) Here too there's no need to add an nsresult which is always NS_OK @@ +766,5 @@ > + dirProps[i] |= IGNORE_CC; > + overflowIsolateCount++; > + } > + break; > + Trailing whitespace @@ +793,3 @@ > case B: > /* > + * We do not expect to see a paragraph separator (B), Maybe put in NS_NOTREACHED, as above @@ +1600,5 @@ > > + int32_t runCount, visualStart, logicalLimit, logicalFirst, i; > + Run iRun; > + > + /* ubidi_countRuns will check VALID_PARA_OR_LINE */ Change the comment to CountRuns @@ +2095,5 @@ > { > if(aVisualIndex<0 || mLength<=aVisualIndex) { > return NS_ERROR_INVALID_ARG; > + } > + Trailing whitespace @@ +2154,4 @@ > nsresult rv; > > /* GetLevels() checks all of its and our arguments */ > + rv = CountRuns(); Change the comment to reflect the change to code. And what is CountRuns() without an argument? @@ +2349,5 @@ > int32_t i, j, destSize; > uint32_t c; > > /* optimize for several combinations of options */ > + switch(options&(UBIDI_REMOVE_BIDI_CONTROLS|NSBIDI_DO_MIRRORING|NSBIDI_KEEP_BASE_COMBINING)) { This can stay NSBIDI_REMOVE_BIDI_CONTROLS, no?

(In reply to Simon Montagu :smontagu from comment #7) > Comment on attachment 8598432 [details] [diff] [review] > > > > /* GetLevels() checks all of its and our arguments */ > > + rv = CountRuns(); > > Change the comment to reflect the change to code. And what is CountRuns() > without an argument? I was racking my brains to understand how this compiled, and of course it's because this part of nsBidi.cpp is #ifdef'd out because we don't use it. It's good that you updated it anyway (it's already happened once or twice that we started using some functions that we hadn't used originally and moved them out of the #ifdef), and it's probably a good idea to try a build with #define FULL_BIDI_ENGINE just to make sure there aren't any other similar issues.

Thanks for the review, Simon! I fixed the nits you mentioned, and I also did a build with FULL_BIDI_ENGINE, which required a few more tweaks but nothing major.

Way to go! Please note that this opens the way to implementing the isolating values of unicode-bidi (isolate, isolate-override, plaintext) by simulating LRI, RLI, FSI, and PDI, as described in http://dev.w3.org/csswg/css-writing-modes-3/#unicode-bidi, and not in the custom way it is implemented currently, thus fixing bugs like 717811.

Yes, I had that in mind as a follow up -- filed bug 1160847

Ted, coverity thinks that there is an important bug in this patch (CID 1296737) On this line: https://dxr.mozilla.org/mozilla-central/source/layout/base/nsBidi.cpp?from=layout/base/nsBidi.cpp#782 coverity says: CID 1296737 (#1 of 1): Out-of-bounds read (OVERRUN)158. overrun-local: Overrunning array stack of 127 2-byte elements at element index 4294967295 (byte offset 8589934590) using index stackLast (which evaluates to 4294967295). I don't know if this tool is correct or not but I am happy to open a new bug if you think it is correct.

Hi Sylvestre, I'm confident that coverity is wrong in this case. I'm guessing that coverity is worried about the fact that lines 775 and 777 appear to decrement stackLast without checking that stackLast >= 0, so that stackLast can wrap-around and become ULONG_MAX. But the contents of the |stack| array should be such that this doesn't happen. (The |stack| array should always contain at least one element greater than ISOLATE when validIsolateCount > 0.) If you want, I can add some sanity checks to the code to stop coverity from complaining. (And this code is rather lacking in sanity checks -- it was copied from a code base that was optimized for speed, not maintainability.) But I don't think it will change the correctness of the code.

(In reply to Ted Clancy [:tedders1] from comment #16) > Hi Sylvestre, > > I'm confident that coverity is wrong in this case. > > I'm guessing that coverity is worried about the fact that lines 775 and 777 > appear to decrement stackLast without checking that stackLast >= 0, so that > stackLast can wrap-around and become ULONG_MAX. But the contents of the > |stack| array should be such that this doesn't happen. (The |stack| array > should always contain at least one element greater than ISOLATE when > validIsolateCount > 0.) > > If you want, I can add some sanity checks to the code to stop coverity from > complaining. (And this code is rather lacking in sanity checks -- it was > copied from a code base that was optimized for speed, not maintainability.) > But I don't think it will change the correctness of the code. Lets add an assertion and a comment there.

(In reply to Gregor Wagner [:gwagner] from comment #17) > Lets add an assertion and a comment there. Okay. Sylvestre, can you create a new bug for this and assign it to me?

Done in bug 1161932. thanks for caring!

This is needed for Bug 1166203, which is a confirmed blocker for 2.2.

Comment on attachment 8599059 [details] [diff] [review] bug-1157726-isolates.patch NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): RTL support for B2G (Bug 906270) This patch is required for Bug 1166203, which is confirmed as blocking 2.2. (And probably a bunch of similar issues which haven't been spotted yet.) User impact if declined: Punctuation marks will appear in the wrong place when LTR phrases appear within RTL text, or vice versa. Testing completed: Green treeherder run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=40fd98cb4711 Risk to taking this patch (and alternatives if risky): None forseen. String or UUID changes made by this patch: None.

Blocking a blocker

Comment on attachment 8599059 [details] [diff] [review] bug-1157726-isolates.patch Hi Ryan, This is the second bug. The next one is bug 1161932