Closed
Bug 1157726
Opened 10 years ago
Closed 10 years ago
Unicode 6.3 Bidi Support - Isolates
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
People
(Reporter: tedders1, Assigned: tedders1)
References
Details
(Whiteboard: [systemsfe])
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
jocheng
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
This is part of Bug 922963.
Assignee | ||
Comment 1•10 years ago
|
||
This is a work-in-progress patch. It builds, but I need to do testing and add unit tests.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8596600 -
Attachment is obsolete: true
Attachment #8597855 -
Flags: review?(smontagu)
Assignee | ||
Comment 3•10 years ago
|
||
Updated•10 years ago
|
Whiteboard: [systemsfe]
Updated•10 years ago
|
Target Milestone: --- → 2.2 S11 (1may)
Assignee | ||
Comment 4•10 years ago
|
||
Added ref test.
Attachment #8597855 -
Attachment is obsolete: true
Attachment #8597855 -
Flags: review?(smontagu)
Attachment #8598203 -
Flags: review?(smontagu)
Assignee | ||
Comment 5•10 years ago
|
||
Fixing compiler warning on Windows.
Attachment #8598203 -
Attachment is obsolete: true
Attachment #8598203 -
Flags: review?(smontagu)
Attachment #8598432 -
Flags: review?(smontagu)
Assignee | ||
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8598432 -
Flags: review?(smontagu) → review+
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 13•10 years ago
|
||
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.
Comment 14•10 years ago
|
||
Yes, I had that in mind as a follow up -- filed bug 1160847
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
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)
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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)
Comment 19•10 years ago
|
||
Done in bug 1161932. thanks for caring!
Updated•10 years ago
|
Flags: needinfo?(sledru)
Assignee | ||
Comment 20•9 years ago
|
||
This is needed for Bug 1166203, which is a confirmed blocker for 2.2.
blocking-b2g: --- → 2.2?
Assignee | ||
Comment 21•9 years ago
|
||
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?
Comment 23•9 years ago
|
||
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+
Updated•9 years ago
|
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
status-firefox38:
--- → wontfix
status-firefox39:
--- → wontfix
Comment 24•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•