fatal error when using workbench access

workbench_access_get_active_tree() should only be called once per page request, unless you are saving data or calling workbench_access_reset_tree().

There is a very dumb typo in the static check.

Bumping to critical. This is rare but showstopping.

That doesn't make sense to me. $active should never return a numeric keyed array. The structure should be:

$active = array(
  'access_scheme' => array(), // with stuff in the array.
  'tree' => array(), // with stuff in the array.
  'active' => array(), // with stuff in the array.
)

So the question is this:

1) Why is your structure different?
2) Why can I not reproduce that structure?

I wonder if it's because the call is erroring out. That's what it looks like. I can only replicate that data structure if I do this:

/**
 * Load the active tree.
 */
function workbench_access_get_active_tree() {
  $active = &drupal_static(__FUNCTION__);
  if (isset($active['access_scheme'])) {
    return $active;
  }
  $scheme = variable_get('workbench_access', 'taxonomy');
  workbench_access_load_include($scheme);
  $func = $scheme . '_workbench_access_info';
  $info = $func();
  $data = $info[$scheme];
  $active = workbench_access_get_ids_by_scheme($data);
 /* $tree = workbench_access_tree($data);
  workbench_access_build_tree($tree);
  $active = array(
    'access_scheme' => $data,
    'tree' => $tree,
    'active' => $active,
  );*/
  return $active;
}

There may be an issue with the re-assignment of the $active variable, so let's try changing that.

Tests pass for this patch, so if it fails fo you, we really need to dig in to what's wrong with your install.

Thanks!

Nope. Cannot replicate. I wonder if it has to do with how Panels is calling the page.

Your original stack trace looked like a Panels page call. Does it occur on all pages or just Panels pages?

Can you export and attach the panels page?

Needs more info, actually, since I cannot replicate without hacking the code.

I'm going to commit the obvious fix. Then we can dig into your issue.

Sure. We did find a nasty and dumb bug!

I'm seeing this on a site running 1.0 (and changing to latest dev doesn't resolve). Attached is relevant portion of the stack trace. Basically, workbench_access_get_active_tree() is recursively calling itself when there is no cache. The issue first started when somebody was saving a node. Specifically, the call to workbench_access_tree() from within workbench_access_get_active_tree() is causing the recursion.

Exact steps to replicate, please.

I still haven't yet figured out how the system got into this state, but I've traced the issue to calls to workbench_access_tree() from within workbench_access_get_active_tree(). The site is using the menu scheme for access, so this function then calls menu_workbench_access_tree(), which in turn calls menu_tree_all_data(), which calls menu_build_tree(), which then calls back to workbench_access_get_active_tree().

I'll provide more information as I find it.

What are you doing when this happens?

Ok, I've traced this back to an essentially recursive link placed in the menu system.

There was a top-level main menu link to node/11, and another link under that pointing to node/11/1. Removing the latter resolved the issue.

I can reproduce this on a stock D7 site running workbench access by putting such a recursive node link into the main menu.

Eek. Not sure we can do much about that.

Yeah, I don't have any immediate ideas on how to get around that, which is unfortunate since this user-error has the power to take down the entire site, with the only fix being some forensics work and the manual removal of the offending link.

We might be able to do some checking in _workbench_menu_build_tree() to ensure that we don't load the same data twice. Perhaps something like so?

/**
 * Recursive helper function to build menus.
 *
 * @param &$tree
 *   The workbench access tree being built.
 * @param $link
 *   The menu link being inspected.
 */
function _workbench_access_menu_build_tree(&$tree, $link) {
  $item = (object) $link['link'];
  if (isset($tree[$item->mlid])) {
    return;
  }
  $tree[$item->mlid] = array(
    'access_id' => $item->mlid,
    'access_type_id' => $item->menu_name,
    'name' => $item->link_title,
    'description' => isset($item->options['attributes']['title']) ? $item->options['attributes']['title'] : '',
    'weight' => $item->weight,
    'depth' => $item->depth,
    'parent' => ($item->plid == 0) ? $item->menu_name : $item->plid,
  );
  if (!empty($link['below'])) {
    foreach ($link['below'] as $below) {
      _workbench_access_menu_build_tree($tree, $below);
    }
  }
}

But that shouldn't be the issue, since node/11 and node/11/1 should have different mlids. I don't really see how this loop occurs.

Does your fatal error call stack ever get to that function?

It doesn't appear to ever reach _workbench_access_menu_build_tree().

We experienced this issue and traced it down to an additional trailing / on a menu entry.

mysql> select * from menu_links where substr(link_path, -1, 1) = '/';
+-----------+------+------+----------------------------------+-------------+------------+-------------------------------------------------------------------+--------+--------+----------+--------------+----------+--------+-------+------------+------+------+----+----+----+----+----+----+----+---------+
| menu_name | mlid | plid | link_path                        | router_path | link_title | options                                                           | module | hidden | external | has_children | expanded | weight | depth | customized | p1   | p2   | p3 | p4 | p5 | p6 | p7 | p8 | p9 | updated |
+-----------+------+------+----------------------------------+-------------+------------+-------------------------------------------------------------------+--------+--------+----------+--------------+----------+--------+-------+------------+------+------+----+----+----+----+----+----+----+---------+
| main-menu | 6389 | 6388 | node/586/                        | node/%      | Overview   | a:1:{s:10:"attributes";a:1:{s:5:"title";s:0:"";}}                 | menu   |      0 |        0 |            0 |        0 |    -50 |     2 |          1 | 6388 | 6389 |  0 |  0 |  0 |  0 |  0 |  0 |  0 |       0 |
+-----------+------+------+----------------------------------+-------------+------------+-------------------------------------------------------------------+--------+--------+----------+--------------+----------+--------+-------+------------+------+------+----+----+----+----+----+----+----+---------+
2 rows in set (0.05 sec)

Fixed with:

update menu_links set link_path = 'node/586' where mlid = 6389;
delete from cache_menu;

I can confirm that editing a menu link and adding a / to the end of it causes this issue.

Well, adding a '/' is user error.

Yes agreed, pity menu system doesn't catch it as an invalid path :(

You should file (or find) an issue for that. That's a UX fail.

imo it's a feature, not a fail.
I've always used trailing args on menu items to facilitate alternate page tpls.

Eg page-node-lightbox.tpl.php has no sidebars etc.

Then links to /node/3/lightbox load with a different page tpl to /node/3

But I agree, the issue is not with workbench_access

But I agree, the issue is not with workbench_access

The problem though is that with just core, the user error can be fixed easily, or ignored. With workbench access enabled, the site fatal errors until the offending path is manually removed from the db.

I am open to fixing this loophole here, if we can figure out how.

This is how I manage to reproduce this error:

1. Enable all workbench* modules.
2. Create a menu with links to administration pages (for me it was just "node/add") and assign this menu to a block visible for all users (even anonymous).
3. Enable workbench moderation & access for nodes with menu access scheme (of course now we have to assign some user to some sections, for me - again - it was section linked with menu item with path "node/add").
4. Access site as anonymous user.

When I follow those steps and clear the cache, if the first page load comes from an anon user, I am getting a page timeout.

There is a race condition in menu_workbench_access_tree() that only appears when certain types of items are placed in the menu. If menu_tree_all_data() has not yet primed the cache, we hit an infinite loop.

Tracked it down.

Two changes in this patch, which tests cleanly with both "node/1/" and "node/add" as "bad" menu items.

• Check to ensure that $name is a valid menu.
• Use _menu_build_tree() directly, which bypasses access checks and eliminates the mystery errors.

I'm not crazy about using a "private" function, but it seems to be stable.

I suspect that this works by bypassing the access check in menu_tree_build(). My concern is that we also bypass translation and may need to call _menu_link_translate().

With menu testing in, this can be reproduced, I believe, by adding 'node/add' into the menu structure test.

The remaining question is whether the solution is proper.

I have tested the latest patch by first reproducing the error and then applying the patch to fix the error.

The error is fixed correctly.

I confirmed the menu items continue to follow the correct access scheme based on permissions.

I will test the translation issue and then this should be ready to be committed.

Re-roll of original patch.

Interesting. The patch in #42 fails without the patch in #44, and perhaps that is a sufficient test condition.

I am replicating the steps in #35.

This patch, which combines #42 and #44 passes.

So the remaining question is simply: Can we safely call the "private" function here?

OK, thanks to @stevector and @Sean Buscay, we were able to replicate this in the test.

Testbot is stalled. This patch is green. Committing.

   7961799..c7e3753  7.x-1.x -> 7.x-1.x