Closed Bug 1157726 Opened 5 years ago Closed 5 years ago

Unicode 6.3 Bidi Support - Isolates

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
2.2 S11 (1may)
blocking-b2g 2.2+
Tracking Status
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: tedders1, Assigned: tedders1)

References

(Blocks 1 open bug)

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 4 obsolete files)

This is part of Bug 922963.
Assignee: nobody → tclancy
Blocks: 922963
Attached patch bug-1157726-isolates.patch (obsolete) — Splinter Review
This is a work-in-progress patch. It builds, but I need to do testing and add unit tests.
Attached patch bug-1157726-isolates.patch (obsolete) — Splinter Review
Attachment #8596600 - Attachment is obsolete: true
Attachment #8597855 - Flags: review?(smontagu)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.2 S11 (1may)
Attached patch bug-1157726-isolates.patch (obsolete) — Splinter Review
Added ref test.
Attachment #8597855 - Attachment is obsolete: true
Attachment #8597855 - Flags: review?(smontagu)
Attachment #8598203 - Flags: review?(smontagu)
Attached patch bug-1157726-isolates.patch (obsolete) — Splinter Review
Fixing compiler warning on Windows.
Attachment #8598203 - Attachment is obsolete: true
Attachment #8598203 - Flags: review?(smontagu)
Attachment #8598432 - Flags: review?(smontagu)
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?
Attachment #8598432 - Flags: review?(smontagu) → review+
(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.
Attachment #8598432 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7d4feaf04bca
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Flags: needinfo?(tclancy)
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.
Flags: needinfo?(tclancy)
(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?
Flags: needinfo?(sledru)
Depends on: 1161932
Done in bug 1161932. thanks for caring!
Flags: needinfo?(sledru)
Depends on: 1162813
Depends on: 1163583
This is needed for Bug 1166203, which is a confirmed blocker for 2.2.
blocking-b2g: --- → 2.2?
Blocks: 1166203
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.
Attachment #8599059 - Flags: approval-mozilla-b2g37?
Blocking a blocker
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8599059 [details] [diff] [review]
bug-1157726-isolates.patch

Hi Ryan,
This is the second bug. The next one is bug 1161932
Attachment #8599059 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Depends on: 1173415
You need to log in before you can comment on or make changes to this bug.