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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(3 files, 5 obsolete files)

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.
Component: Layout: Text → Graphics
OS: Mac OS X → All
QA Contact: layout.fonts-and-text → thebes
Hardware: x86 → All
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.
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.
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)
Looks like the right approach to me!
Attachment #559967 - Flags: review?(jdaggett)
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)
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)
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)
Tryserver build with these patches is available at
https://tbpl.mozilla.org/?tree=Try&rev=91b2ee4b4904
Oops, that failed to build on Windows because I dropped the OTS_API from the exported function. Fixed now.
Attachment #623955 - Flags: review?(jdaggett)
Attachment #623682 - Attachment is obsolete: true
Attachment #623682 - Flags: review?(jdaggett)
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)
Rebased; review ping?
Attachment #623683 - Attachment is obsolete: true
Attachment #623683 - Flags: review?(jdaggett)
Attachment #627217 - Flags: review?(jdaggett)
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.
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 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+
Attachment #627217 - Flags: review?(jdaggett) → review+
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.
(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.)
(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'.
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)
Attachment #627907 - Flags: review?(jdaggett) → review+
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
You need to log in before you can comment on or make changes to this bug.