happy bmo push day!

release tag

the following changes have been pushed to bugzilla.mozilla.org:

  • [1433993] Outdated FreeOTP link in user preferences
  • [1433833] Add index to email_rates.message_ts
  • [1436301] Exempt bot accounts from idle group removal
  • [1430259] Update policy code in BMO PhabBugz extension to update custom policy if a private bugs groups have changed.
  • [1343248] Migrate secbugstats scripts to bmo production
  • [1434064] Refactor Project.pm to use Moo for better type checking
  • [1434438] Refactor Revision.pm to use Moo for cleaner type checking
  • [1424363] securemail help page recommends getting a certificate from StartCom
  • [1433400] Prevent cross-site image requests from leaking contents of certain fields due to regex search
  • [1435735] Add script to add email job to jobqueue
  • [1211377] needinfo from someone not in sec group shows warning message even when bug is being removed from sec group
  • [1432296] Prevent bugzilla static assets from being blocked by overly long request URIs

discuss these changes on mozilla.tools.bmo.

Profiling as validation

Among a bunch of other things that are going on, we’re migrating bugzilla.mozilla.org to a new home in AWS.

So the team (bobm and ckolos) have been very dedicated to validating the new stuff is as good, and hopefully better than the old stuff. To this end they’ve been working with another engineer (rpapa) to do load testing. Some of the load testing results have been a bit unusual, perhaps even impossible.

But that’s okay, because I’ve recently made it very easy to profile the code using Devel::NYTProf.
Looking at preliminary profile data, it seems that perhaps the overhead of connecting to mysql may be higher in the new environment.
It could also be something else — more analysis is needed.

I’m sure this must be a thing, but to be honest I haven’t ever read about someone using a profiler in this way, so perhaps is deserves a mention.

My Little Features: Deprecation is Magic

I’m still hammering out what features I want to see land in BMO this year but one thing I’ve come to realize is that often the way forward on an old code base is through careful deprecation.

Adding new features, or even fixing existing bugs is often blocked by the immense weight of past decisions.

Recently we deprecated support for IE 11.
This was magic: We can use more modern javascript features. async/await, arrow functions, and so on make the frontend code much nicer to work with. It puts us on a good footing to remove our use of legacy JS frameworks and (I believe) makes contributions more attractive.

This is not to say we will enthusiastically deprecate everything that holds us back, but a measured approach is called for here.

I’m going to list the things that I intend to deprecate in the first half of 2018. This isn’t a roadmap for 2018 but these deprecations and changes will feed into that.

Server-side bug link tooltips

Right now we spend a lot of time rendering comments.
It would be nice to cache them — but they’re not cache-able in general because they contain user-specific data. That is, if you can see a security bug you’ll be able to see its description in the mouse-over hover of a bug.

After looking at several options for this, I realized it would be better to add the tool-tips client side.

While not a silver bullet, this also paves the way for adopting markdown (something I’m actively working on).

Benefit: Faster page loads, especially on popular bugs with more than a few dozen comments.

Server-side PUSH used in buglist.cgi

So if you use Firefox, you get a much better experience using buglist.cgi. You see the chomping dinosaur while your search results are collected. Unfortunately, this doesn’t work in any other browser, and it isn’t standardized. As a result, we have to be very careful with our load balancers and so on.

Making this fetch() call is pretty easy, but supporting both as an in-page fetch() and an HTTP push is quite hard. So when this switches to a fetch() the HTTP push is going away.
This is almost not a deprecation because the functionality will be mostly the same.

Benefit: Firefox users will get a better UX when request times out, and everyone else will benefit too.

The legacy show bug view will lose inline history

Right now 90% of page views hit a page that uses CSP. But many people
continue to use the legacy HTML view, which cannot do CSP because of the
InlineHistory Bugzilla extension. The legacy bug page is deprecated already
— it does not receive updates and we don’t care much about UX regressions
on it.

Short of just turning it off, I’m going to enable CSP for it. This means
inline history won’t work.

At some point, we’re just going to turn it off, and this is a step towards that.

Benefit: Security, less bad javascript to maintain.

Unsafe links will no longer be linkified

Already javascript: links in the URL field do not work on the main bug view
page. Now all bug URL links that are marked “unsafe” will appear as
clipboard-copyable text areas.

Benefit: Usability, you’ll be able to more easily use test-cases that are javascript: links.

happy bmo push day!

release tag

the following changes have been pushed to bugzilla.mozilla.org:

  • [1429600] Update Revision.pm type checking to treat bug id as a simple string or undefined
  • [1426414] Send preload headers for webfonts
  • [1431135] add bug status css class to markup
  • [1430495] Make loading of Requests dropdown faster
  • [1429785] In-page back navigation broken
  • [1429688] focus Search Bugs box by default
  • [1431294] requests.cgi – allocate less memory
  • [1428270] Unwrappable content in summary can cause top buttons to bleed out of main content box
  • [1429888] See All link in Requests dropdown should show results grouped by flags as before
  • [1432812] Send preload headers only for web-browser requests

discuss these changes on mozilla.tools.bmo.

The Binding of libcmark-gfm: Segfaults and Debugging

So for various reasons — including “I want bugzilla.mozilla.org to support markdown” I have been working to get a binding to github’s fork of cmark.

For the first part of this, I got some help in #native
writing a Perl module in the Alien:: namespace,
namely Alien::libcmark_gfm.

Armed with this module, I’ve been seeking to make CommonMark
work against the GitHub fork of libcmark.

So far things have been going well, and I decide to just be dumb. The API for libcmark-gfm is a bit different, so I’ll rename the packages from CommonMark to CommonMarkGFM.

Of course, this was the first problem: I was getting
errors about a package not existing, a package named CommonMarkGFM::N. What the hell does that mean? I haven’t changed much yet!

The problem was this bit of C code in the newly-renamed CommonMarkGFM.xs:

stash = gv_stashpvn("CommonMarkGFM::Node", 16, GV_ADD);

Okay, now I don’t know perlguts very well.
I don’t know what gv_stashpvn does (but I can find the docs for gv_stashpvn
and the name is a hint at what it does, in the terse nomenclature of Perl’s internal APIs)

The old string was 16 bytes long. Now it should be 19,
and that perfectly explains why I saw CommonMarkGFM::N.

So I get past that. and now the test suite segfaults.

ok 1 - use CommonMarkGFM;
ok 2 - markdown_to_html
ok 3 - 'parse_document' isa 'CommonMarkGFM::Node'
Segmentation fault

Hey, maybe this is the same as the first problem I fixed?

So I go looking for that problem, and I find it!
We have some lengths hard-coded in the typemap file
(no, aside from the fact it maps types, I don’t know what the typemap file does. I’m not usually hacking in perlapi).

    $var = (cmark_node*)S_sv2c(aTHX_ $arg, \"CommonMarkGFM::Node\", 19, cv,
/* more omitted */

So I fix those problems, but they were not my problem.
I’m still getting a segfault…

I’m really quite excited at this moment! I have a problem that I can apply things I learned about from this wonderful blog by Julia Evans.

I’ve already been using a Dockerfile to try to compile and test this code so I just need to install Valgrind (and maybe gdb too) and see what happens.

So I run valgrind:

==16==  Access not within mapped region at address 0x88
==16==    at 0xEF4685C: cmark_render_html_with_mem (in /usr/local/lib64/perl5/auto/share/dist/Alien-libcmark_gfm/lib/libcmark-gfm.so.0.28.3.gfm.12)
==16==    by 0xED0A11D: XS_CommonMarkGFM__Node_interface_render (CommonMarkGFM.c:898)
==16==    by 0x4ED6814: Perl_pp_entersub (pp_hot.c:2888)
==16==    by 0x4ED4B05: Perl_runops_standard (run.c:40)
==16==    by 0x4E7D0D7: perl_run (perl.c:2435)
==16==    by 0x400E73: main (perlmain.c:117)

Huh, interesting. Okay, maybe I can use gdb to set a breakpoint there.

(gdb) b cmark_render_html_with_mem
Function "cmark_render_html_with_mem" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (cmark_render_html_with_mem) pending.

Our function doesn’t exist yet as it’s in a shared object that will get loaded later. This is fine — except it isn’t. My breakpoint never happens.

Huh! I guess (as it turns out, wrongly) that maybe I need to change my compilation options. And I also assume the segfaulting is because of something in the Perl extension code.

So maybe it’s that we compile with -02. My gcc is too old to support -Og, so let’s try -O0.

At this point, I’m just copying the line from make’s output and changing it. I just want to get some details in gdb damn it!

So I run the following:

perl Makefile.PL
gcc -c  -I/usr/local/lib64/perl5/auto/share/dist/Alien-libcmark_gfm/include -D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O0 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic   -DVERSION=\"0.280301\" -DXS_VERSION=\"0.280301\" -fPIC "-I/usr/lib64/perl5/CORE"   CommonMarkGFM.c
make install

Now I can run perl t/03_render.t again, under gdb, and see if I can get more details.

ok 1 - use CommonMarkGFM;
ok 2 - markdown_to_html
ok 3 - 'parse_document' isa 'CommonMarkGFM::Node'
ok 4 - parse_document works
ok 5 - render_xml
ok 6 - render_man
ok 7 - render_latex
ok 8 - render_commonmark
ok 9 - render functions return encoded utf8
ok 10 - render functions expect decoded utf8

My attitude thus far is clear in the tweet that followed:

Now I proceed to have fun.

I spent the time trying to figure out what -O1 vs. -O0 did, and I wrote a script to repeatedly re-compile that one file
with different options. Along the way, I learned how to make gcc spit out what options it is compiling with (gcc -Q -v ...).
I had some false positives, and then I went to sleep.

After a period of sleep, figured out I wanted the list of flags as a difference between -O0 and -O1. I cleaned up my compile.pl script
and ran it.

The answer is: all of them are fine. -O0 and all the feature flags of -O1 result in no segfault either. Adding -O1 back brings back the segfault. After some more searching of the gcc docs, it is implied some optimizations are just directly tied to the O level.

My fun is now over, and I’ll do the more boring task of figuring out why my code is broken.

Staring at my from gdb’s output is this:

warning: Error disabling address space randomization: Operation not permitted

After a bit of searching, I find a fix for this to run the docker image
with --security-opt seccomp=unconfined.

And suddenly, breakpoints work.

and I can debug the root variable that is passed to cmark_render_html_with_mem… and nothing is wrong there.
Probably I need to re-compile libcmark-gfm with more debugging, I think. Suddenly, I realize that cmark_render_html_with_mem takes three arguments, and the Perl XS code is only passing it two.
How does this work? Well, it appears to cast a pointer to a function pointer, and call it. Calling a function pointer with fewer arguments than it is declared to with is undefined behavior, and I guess the rest of the behavior I observed was nasal demons.

(as an FYI, this argument difference is an API change between upstream libcmark and libcmark-gfm).

Finally, this third argument is a linked list of syntax extensions,
and it’s not clear yet how I will need to pass that back and forth between perl and C. This is also indicative that CommonMarkGFM will need to be a fork of CommonMark