happy bmo push day: now with added contrast

release tag

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

  • [1225902] Show only flags with requestee in the “Flags You Have Requested” section
  • [1552720] Linkify bug summaries on My Dashboard query table
  • [1542554] Add bug type icons to dependency trees
  • [1514000] Suppress duplicated changes in bug history made at the same time mainly due to mid-air collisions
  • [1523536] New bug’s “Choose from your most-used components” list is slow to show up
  • [1538115] Add shortcuts for tracking & status flags
  • [1553893] Remove horizontal rule from summary section as well as when email notifications are sent
  • [1552885] Fix issues in the post-Sandstone skin including low contrast, visited links and small font size
  • [1283312] Advanced Search page doesn’t list Flags and many other fields in Search By Change History
  • [1543489] Update firefox-crash-table.js to use cached firefox_versions.json
  • [1553780] Can’t type/paste text into attachment contents and set text/html mimetype
  • [1546437] group reviewers not properly flagged in “Phabricator Revisions” bugzilla section

discuss these changes on mozilla.tools.bmo.

happy bmo push day!

release tag

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

  • [1511490] BMO’s oauth tokens should be use jwt
  • [1519782] The OrangeFactor extension should link back to Intermittent Failure View using ‘&tree=all’
  • [1523004] Sort Phabricator revisions by numeric value instead of alphabetically
  • [1523172] Advanced Search link on home page doesn’t always take me to Advanced Search
  • [1523365] Ensure all requests have the HSTS header (if configured)
  • [1433080] No longer show the template for tracking & release notes requests
  • [1522731] When you click “Update comment”, the button changes size and the “Cancel” button jumps underneath your cursor, causing momentary panic that you canceled your edit
  • [1512815] Optimize Bugzilla->active_custom_fields() for CPU and memory usage

discuss these changes on mozilla.tools.bmo.

happy bmo push day!

release tag

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

  • [1511261] request queue page shows ‘Bugzilla::User=HASH(…)’ instead of username
  • [1520856] “Opt out of these emails” at bottom of overdue request nagging emails doesn’t open desired page
  • [1520011] Phabbugz panel short description missing
  • [1518886] Remove outdated build plan code from PhabBugz extension used to move revisions from draft mode.
  • [1509329] Do not display revisions that have moved out of the bug, but note the move in the bug history
  • [1520533] Utilize Markdown in uplift form comments
  • [1521653] Cannot edit comments after creating or updating an attachment
  • [1518268] Re-style all markdown content, consistently
  • [1520202] Sometimes the browser can cache the wrong version of an asset
  • [1517429] Search: Filter out by default Product containing “Graveyard”
  • [1512815] Optimize Bugzilla->active_custom_fields() for CPU and memory usage
  • [1520582] Block ips of users that get too many page errors
  • [1522155] Closed bug links don’t get their strike-through

discuss these changes on mozilla.tools.bmo.

happy bmo push day!

release tag

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

  • [1467297] variable masks earlier declaration in Feed.pm in Phabbugz extension
  • [1467271] When making a revision public, make the revision editable only by the bmo-editbugs-team project (editbugs)
  • [1456877] Add a wrapper around libcmark_gfm to Bugzilla
  • [1468818] Re-introduce is_markdown to the longdescs table (schema-only)
  • [1469689] Remove Bugzilla Helper and custom bug entry form links from Browse page
  • [1419971] Add new Developer Tools and WebExtensions products to easy product selector on Browse and Enter Bug pages
  • [1469827] The etiquette check on “Create new a Bugzilla account” lacks a proper label
  • [1469920] Update schema: add a nickname to profiles table and a fulltext index on the profiles realname field
  • [1469333] Check attachment file size client-side and inform user of too large file before uploading it
  • [1461379] API DB Availability Exceptions on recurring BMO scripts
  • [1393146] Automate blocking IPs that bugzilla flags as exceeding rate limits
  • [1470275] Copy Summary button should give some feedback
  • [1470343] GitHub PR diff is not decoded in UTF-8
  • [1470485] Create new policies using PhabricatorProjectsAllPolicyRule instead of PhabricatorProjectsPolicyRule
  • [1469881] Patches posted by Phabricator to Bugzilla don’t list the patch author
  • [1457900] When restricting a revision to a bugzilla group we should tag the revision with the project
  • [1471044] Allow some model classes to have dynamic column names with class method DYNAMIC_COLUMNS
  • [1470966] “Status” column in Phabricator dashboard isn’t very useful
  • [1452096] Some custom dropdown UI widgets stay fixed and don’t move with scroll
  • [1471304] Block sending mail to hosts that end with .tld or .bugs
  • [1457550] Update scripts/remove-non-public-data.pl suitability for current BMO infrastructure.
  • [1469023] Show “new changes since (datetime)” indicator that links to unread changes/comments

discuss these changes on mozilla.tools.bmo.

happy bmo push day!

release tag

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

  • [1459336] feed daemon skips setting r+ for accepted revision if the same user already has a flag set even if flag is status of ?
  • [1460466] Phab bot does not create r+ for acceptance when there are still blocking reviewers
  • [1440086] Refactor PhabBugz extension code to use new User.pm module for better type checking
  • [1458664] Feed daemon when adding or updating a new project in Phabricator, it should fix permissions and also make sure phab-bot is project member
  • [1462686] Current phabbugz in bmo master still refers to get_phab_bmo_ids() which is no longer part of the code
  • [1461819] Plack::Handler::Apache2 accidentally unsets $ENV{MOD_PERL}
  • [1461400] Log errors in webservices when undef values are passed to $self->type()

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.

1..10
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).

T_NODE
    $var = (cmark_node*)S_sv2c(aTHX_ $arg, \"CommonMarkGFM::Node\", 19, cv,
                               \"$var\");
/* 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
make
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.

1..10
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

Bigger and better search box on BMO

There is a lot of power hidden in quick search and my data suggests it is under-utilized.

For instance, searching for all review flags is the literal search tag review?
Similarly you can do needinfo?dylan@mozilla.com to find all bugs with needinfos directed at me (actually, this query performs a slightly broader search but I have code to fix that).

There are dozens more examples. The quick search help is a long read, and most people won’t bother.

A long time ago glob suggested stealing the UI from DXR, where you get a little quick-help on the operator
syntax for DXR searches. It’s a pretty simple change, right?

Well, our search box is small. So the first thing it needs is to be bigger.

bigger quicksearch box

More room to work with. This required replacing the table-based layout with some flex boxes. The top-line is nearly pixel perfect
to its previous table-based implementation.

We can also hide some things and begin making the UI responsive

portrait view of bmo quicksearch.

I hope to post a followup showing the quicksearch syntax helper,
but this is at the moment just a side task.

(Although it ties well into the goal of implementing elasticsearch on BMO).

Sorry, I meant I changed it from 226s to 184ms

About twenty days ago it came to my attention via my colleague Ed Morley that BMO’s bzapi was very slow.
It turns out he had reported the same issue the prior year as well!

Performance problems are very enjoyable to work on, I find. Especially when they are reproducible.

I lost most of the day on this, but in the end I was able to take the slowest function from executing at a leisurely 226 seconds to a very fast 184 milliseconds

Fixed some memory leaks in bugzilla.mozilla.org

So last week I fixed Bug 1282606 which has resulted in a bit of a performance improvement for bugzilla.mozilla.org:

Restarts per Hour

Apache2::SizeLimit
is configured to kill processes once they use more than 700M and this happened about every 7 minutes.

About two weeks ago, while working on some performance issues relating to BMO’s new show_bug ui, I discovered that the problem could get worse: running out of memory every 60 seconds. Should everyone switch to the new UI (which is intended to put less load on the server) a lot more load would be on the server. That’s pretty bad, since we want everyone using the new UI as soon as possible. 🙂

This memory leak isn’t new, and I had filed an investigatory bug about it last year. Memory leaks in perl are caused by having cyclic references, and the solution is to not have cycles, use weak references, or to break the cycle when you’re done with whatever data structure it is part of.

I understand the problem, and I know how to fix it… but maybe I don’t know where the problem is?

Thankfully there is a tool for this on CPAN: Devel::MAT.

Using Devel::MAT, it is possible to dump the address space of a perl program and explore it in great detail in a GUI.

I didn’t set out to remove all the memory leaks this time, just the ones that were the biggest or grew the fastest. This meant the TrackingFlags extension Flag objects, the Bug object, and the Comment object.

The changes are on github for the curious,
and the resulting charts below speak for themselves.

Average Request Time

Requests Before Restart

Age of Process Before Restart