Closed
Bug 670901
Opened 13 years ago
Closed 12 years ago
when the OTS sanitizer rejects a font, we should log a message indicating the specific reason for failure
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: jfkthame, Assigned: jfkthame)
Details
Attachments
(3 files, 5 obsolete files)
5.63 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
8.85 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
28.78 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
Followup to bug 494130. Currently, when OTS blocks a font, we just report "rejected by sanitizer". It would be more helpful to font developers (and to authors who need to report problems to their font supplier) to identify the specific reason for rejection. This will require modifications to the OTS code, which does not currently return useful error codes or messages.
Updated•13 years ago
|
Component: Layout: Text → Graphics
OS: Mac OS X → All
QA Contact: layout.fonts-and-text → thebes
Hardware: x86 → All
Comment 1•13 years ago
|
||
Hoping this might be the correct place to jump in on this thread about OTS. Has anyone indicated that a OTS utility might be a good idea. Something that Foundries, Designers and others can use to run batches of fonts through instead of test each one in Firefox or use some compiled Linux version. Or does this exist and I am missing something? I think there is a large community that exists who would benefit from having some tool to assist us.
Comment 2•13 years ago
|
||
Yes, having a command-line utility that is built from OTS source would be ideal. The first step is to get reasonable error messages, at least something that identifies the problematic table(s) within a font.
Assignee | ||
Comment 3•13 years ago
|
||
Here's a possible approach that I think we could implement without too much difficulty, and perhaps get accepted upstream (I hope). This implements messages that are hard-coded (in English) in the OTS source, comparable to the "warning" messages that are already present. I don't think it is worth the added infrastructure to make these localizable at present; these are technical messages relating to OpenType font structure, which require an understanding of the OpenType spec (which AFAIK is only provided in English) in order to be useful. Little if any of the technical information or tools that relate to font development and validation are likely to be localized into non-English languages, so localizing the error messages here would provide no real value. (I also note that many of the other messages we print to the error console are similarly non-localizable.) For now, the implementation here is conditional on a compile-time #define, MOZ_OTS_REPORT_ERRORS. If this is undefined, the messages will be discarded by the compiler and the OTS API is unchanged. If MOZ_OTS_REPORT_ERRORS is defined, however, then the Process() function takes two additional arguments, a message-reporting function pointer and a userdata pointer. In the main OTS source, the strategy is simply to examine each instance of OTS_FAILURE(), and replace most of them with an appropriate OTS_FAILURE_MSG("message stating what went wrong"). In cases where OTS_FAILURE() is simply "passing up" a failure status that was detected at a lower level, or detecting a low-level read error that will then result in a parse failure, it is left unchanged (and therefore silent) to avoid spamming the console with multiple messages from different levels of the code. In this patch, I've only added parser messages to the head.cc file; the other tables don't yet report their errors. Requesting review at this stage for the overall approach - if this seems reasonable, I'll work through more of the tables, and discuss with the upstream OTS people.
Attachment #559967 -
Flags: review?(jdaggett)
Comment 4•13 years ago
|
||
Looks like the right approach to me!
Updated•13 years ago
|
Attachment #559967 -
Flags: review?(jdaggett)
Assignee | ||
Comment 5•12 years ago
|
||
Excerpted and rebased from the original patch here; adds an error-reporting callback to OTS, conditionalized on MOZ_OTS_REPORT_ERRORS (although I hope it will eventually be accepted upstream as an unconditional feature). Only the top-level parsing code in ots.cc actually reports errors, so any failures within a specific table are just reported as a generic "failure to parse table XXXX". More precise details could in principle be provided by each parser, if we take the time to write all those specific messages.
Assignee: nobody → jfkthame
Attachment #559967 -
Attachment is obsolete: true
Attachment #623682 -
Flags: review?(jdaggett)
Assignee | ||
Comment 6•12 years ago
|
||
This cleans up some typos in the existing OTS warning messages, and adds missing warnings to the gdef parser where it decides to "drop" the table, for consistency with the gsub and gpos parsers. I expect this to go upstream as well.
Attachment #623683 -
Flags: review?(jdaggett)
Assignee | ||
Comment 7•12 years ago
|
||
With the OTS infrastructure from the preceding patches, this actually enables error reporting in our build. Many of the messages can be seen in the Web Console by loading the various "broken-font" testcases from http://dev.w3.org/webfonts/WOFF/tests/UserAgent/Tests/xhtml/.
Attachment #623684 -
Flags: review?(jdaggett)
Assignee | ||
Comment 8•12 years ago
|
||
Tryserver build with these patches is available at https://tbpl.mozilla.org/?tree=Try&rev=91b2ee4b4904
Assignee | ||
Comment 9•12 years ago
|
||
Oops, that failed to build on Windows because I dropped the OTS_API from the exported function. Fixed now.
Attachment #623955 -
Flags: review?(jdaggett)
Assignee | ||
Updated•12 years ago
|
Attachment #623682 -
Attachment is obsolete: true
Attachment #623682 -
Flags: review?(jdaggett)
Assignee | ||
Comment 10•12 years ago
|
||
Rebased to current trunk code, following OTS update. Review ping?
Attachment #623955 -
Attachment is obsolete: true
Attachment #623955 -
Flags: review?(jdaggett)
Attachment #627216 -
Flags: review?(jdaggett)
Assignee | ||
Comment 11•12 years ago
|
||
Rebased; review ping?
Attachment #623683 -
Attachment is obsolete: true
Attachment #623683 -
Flags: review?(jdaggett)
Attachment #627217 -
Flags: review?(jdaggett)
Comment 12•12 years ago
|
||
I've looked at this but I haven't had a chance to sit down and write some tests for this. The code looks fine but I'm wondering if it can be structured in a way such that the command-line tool for OTS spits out the warning messages also. If that's a lot more work, then at least I think that the conditionals shouldn't be MOZ_xxx conditionals but OTS conditionals that are off by default and enabled in our makefile. Or at least have the high-level MOZ conditional enable the OTS conditional, something like that. This would allow us to evangelize the use of the command-line tool to folks like Typekit and WebINK so that they could run it and figure out feature-related bugs themselves without needing to ping us when features don't seem to work due to the sanitizer.
Assignee | ||
Comment 13•12 years ago
|
||
This would depend on getting the error-reporting functionality merged into upstream OTS. I opened an issue about this (http://code.google.com/p/chromium/issues/detail?id=97467), and have just posted an updated patch there, along with a patch to add error reporting to the 'idempotent' command-line tool, so I hope this will get merged before too long. The patch I've posted upstream adds the error-message callback unconditionally (though it defaults to NULL, so clients don't have to change if they don't want the feature); I hope OTS will accept it in that form, for simplicity. The MOZ_OTS_... conditional was not expected to be a long-term part of the solution, it just serves for now to distinguish the extension we're adding to the code from upstream.
Comment 14•12 years ago
|
||
Comment on attachment 623684 [details] [diff] [review] part 3 - add OTS message callback and enable error reporting in our build > +#ifdef MOZ_OTS_REPORT_ERRORS > + OTSCallbackUserData userData; > + userData.mFontSet = this; > + userData.mProxy = aProxy; > +#define ERROR_REPORTING_ARGS &gfxUserFontSet::OTSMessage, &userData > +#else > +#define ERROR_REPORTING_ARGS > +#endif > + > + if (ots::Process(&output, aData, aLength, > + ERROR_REPORTING_ARGS, > + PRESERVE_GRAPHITE)) { I don't think the !MOZ_OTS_REPORT_ERRORS case works here, you'll end up with ',,' which won't compile. I think you want to put the comma in the definition of ERROR_REPORTING_ARGS.
Attachment #623684 -
Flags: review?(jdaggett) → review+
Updated•12 years ago
|
Attachment #627217 -
Flags: review?(jdaggett) → review+
Comment 15•12 years ago
|
||
Comment on attachment 627216 [details] [diff] [review] part 1 v3 - add error-message callback mechanism to OTS > +#ifdef MOZ_OTS_REPORT_ERRORS > +// signature of the function to be provided by the client in order to report errors; > +// this should normally return 'false', unless you want to try to continue processing > +// after an error is detected (which may or may not be safe to do!) > +typedef bool (*MessageFunc)(void *user_data, const char *format, ...); > +#endif Um, what?!? Describe what the semantics of the return value clearly first, then comment. > +#ifdef MOZ_OTS_REPORT_ERRORS > +#define OTS_FAILURE_MSG_HDR(hdr,msg) \ > + ((hdr)->message_func && \ > + (*(hdr)->message_func)((hdr)->user_data, \ > + "%s", \ > + msg)) > +// These macros are defined in ots.h for convenience in the table-specific files; > +// we redefine them here because in this source, 'file' is not the right > +// variable to refer to the current OpenTypeFile; we need 'header' instead. > +#undef OTS_FAILURE_MSG > +#define OTS_FAILURE_MSG(msg) OTS_FAILURE_MSG_HDR(header,msg) > +#undef OTS_FAILURE_MSG_TAG > +#define OTS_FAILURE_MSG_TAG(msg, tag) \ > + (header->message_func && \ > + (*header->message_func)(header->user_data, \ > + "table '%s': %s", \ > + tag, msg)) > +#else > +#define OTS_FAILURE_MSG_HDR(hdr,msg) OTS_FAILURE() > +#endif This is really confusing to follow. If you need to define another macro, do that, don't swizzle around the definition of an already-defined macro. > +#define OTS_FAILURE_MSG_0(msg) \ > + (file->message_func && \ > + (*file->message_func)(file->user_data, \ > + "%s", \ > + msg)) This doesn't seem to be used anywhere. Trim.
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to John Daggett (:jtd) from comment #14) > I don't think the !MOZ_OTS_REPORT_ERRORS case works here, you'll end > up with ',,' which won't compile. I think you want to put the comma > in the definition of ERROR_REPORTING_ARGS. Good point, will fix. (Actually, I'm hoping to get the feature accepted upstream without a compile-time switch, as there's no significant cost if the client doesn't want to provide a callback. If that happens, this clutter will all go away.)
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to John Daggett (:jtd) from comment #15) > Comment on attachment 627216 [details] [diff] [review] > part 1 v3 - add error-message callback mechanism to OTS > > > +#ifdef MOZ_OTS_REPORT_ERRORS > > +// signature of the function to be provided by the client in order to report errors; > > +// this should normally return 'false', unless you want to try to continue processing > > +// after an error is detected (which may or may not be safe to do!) > > +typedef bool (*MessageFunc)(void *user_data, const char *format, ...); > > +#endif > > Um, what?!? Describe what the semantics of the return value clearly first, > then comment. Well, the return value was destined to become the value of the OTS_FAILURE_* macro, which if 'false' causes the current parser to return and is detected as an error by the caller. But the more I think about it, the more I'm convinced that there's no reasonable use-case for OTS_FAILURE() to evaluate to anything _other_ than false, as it's used in lots of places where continuing would result in out-of-bounds reads (or worse). So I'm reworking this a bit such that the result of the MessageFunc will be ignored, and failures will always terminate parsing (of the current table) immediately. OTS simply isn't designed to continue looking for additional errors in a given table once it has detected a serious fault, which would theoretically be the aim of returning 'true'.
Assignee | ||
Comment 18•12 years ago
|
||
OK, here's a tidier and less confusing version. Checked that (with the comma fix from comment 14 in part 3) this builds and runs as expected both with and without MOZ_OTS_REPORT_ERRORS.
Attachment #627216 -
Attachment is obsolete: true
Attachment #627216 -
Flags: review?(jdaggett)
Attachment #627907 -
Flags: review?(jdaggett)
Updated•12 years ago
|
Attachment #627907 -
Flags: review?(jdaggett) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/df4cf37667ac https://hg.mozilla.org/integration/mozilla-inbound/rev/cd033f175e87 https://hg.mozilla.org/integration/mozilla-inbound/rev/664a8855fde5
Target Milestone: --- → mozilla15
Comment 20•12 years ago
|
||
backed out due to test failures / crash: TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/gfx/tests/crashtests/580100-1.html | Exited with code 1 during test run INFO | automation.py | Application ran for: 0:02:27.889205 INFO | automation.py | Reading PID log: /tmp/tmphtva77pidlog ==> process 2187 launched child process 2691 INFO | automation.py | Checking for orphan process with PID: 2691 Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64/1338331614/firefox-15.0a1.en-US.linux-x86_64.crashreporter-symbols.zip PROCESS-CRASH | file:///home/cltbld/talos-slave/test/build/reftest/tests/gfx/tests/crashtests/580100-1.html | application crashed (minidump found) Crash dump filename: /tmp/tmpgxzofn/minidumps/72fd031b-5476-e84e-571dd5a3-0203cc45.dmp Operating system: Linux 0.0.0 Linux 2.6.31.5-127.fc12.x86_64 #1 SMP Sat Nov 7 21:11:14 EST 2009 x86_64 CPU: amd64 family 6 model 23 stepping 10 2 CPUs Crash reason: SIGSEGV Crash address: 0x544c4350 Thread 0 (crashed) 0 libc-2.11.so + 0x474b1 rbx = 0xbb4909c0 r12 = 0x00000007 r13 = 0xbb490d20 r14 = 0x544c4350 r15 = 0xffffff78 rip = 0xd2e474b1 rsp = 0xbb490300 rbp = 0xbb4909b0 Found by: given as instruction pointer in context 1 libxul.so!nsCSSFrameConstructor::ProcessRestyledFrames [nsCSSFrameConstructor.cpp : 8011 + 0x7] rip = 0xbf65e774 rsp = 0xbb490310 Found by: stack scanning 2 0xbf650396 rbx = 0xffffffff r12 = 0xbb4905b0 r13 = 0xbb490420 r14 = 0xa1e40bf0 r15 = 0x00000000 rip = 0xbf650397 rsp = 0xbb490410 rbp = 0x00000000 Found by: call frame info 3 libxul.so!nsGenericElement::Release [nsGenericElement.cpp : 5055 + 0x4] rip = 0xbf80eeae rsp = 0xbb4904b0 Found by: stack scanning 4 0x7fffbb4905af rbx = 0xa5fc25e0 rip = 0xbb4905b0 rsp = 0xbb4904d0 Found by: call frame info 5 libxul.so!nsGenericElement::Release [nsGenericElement.cpp : 5055 + 0x4] rip = 0xbf80eeae rsp = 0xbb4904e0 Found by: stack scanning 6 libxul.so!nsTArray<mozilla::css::RestyleTracker::RestyleEnumerateData, nsTArrayDefaultAllocator>::DestructRange [RestyleTracker.h : 105 + 0x4] rbx = 0xbb4905b0 rip = 0xbf65076d rsp = 0xbb490500 Found by: call frame info 7 0x7f7b9a0f84f7 rbx = 0xbf525f5a rip = 0x9a0f84f8 rsp = 0xbb490520 rbp = 0xbb490580 Found by: call frame info 8 libxul.so!nsCSSFrameConstructor::EndUpdate [nsCSSFrameConstructor.cpp : 8238 + 0x4] rip = 0xbf652b83 rsp = 0xbb490540 Found by: stack scanning 9 libxul.so!mozilla::css::RestyleTracker::DoProcessRestyles [sps_sampler.h : 151 + 0x9] rbx = 0x9a0f84e8 rip = 0xbf650ca2 rsp = 0xbb490550 Found by: call frame info 10 0xfffffff rbx = 0xbffe034a r12 = 0x00000000 r13 = 0x00000000 r14 = 0xa21e2000 r15 = 0x00008d0d rip = 0x10000000 rsp = 0xbb491210 rbp = 0xc118a000 Found by: call frame info 11 libxul.so!gfxUserFontSet::LoadFont [gfxUserFontSet.cpp : 675 + 0x24] rip = 0xbffe0ebf rsp = 0xbb491260 Found by: stack scanning https://tbpl.mozilla.org/php/getParsedLog.php?id=12173514&tree=Mozilla-Inbound
Assignee | ||
Comment 21•12 years ago
|
||
Drat, I broke this when rebasing after the OTS update in bug 747816. ( Re-landed with the crash fixed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b38f193ea6ef https://hg.mozilla.org/integration/mozilla-inbound/rev/e5bce1abf464 https://hg.mozilla.org/integration/mozilla-inbound/rev/d88ec1a7eaf6
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b38f193ea6ef https://hg.mozilla.org/mozilla-central/rev/e5bce1abf464 https://hg.mozilla.org/mozilla-central/rev/d88ec1a7eaf6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•