Page MenuHomePhabricator

Javelin behaviors which pass PHUIViews to configuration parameters fatal the page in an abrupt way
Open, WishlistPublic

Description

Lowest priority and not a recent regression, just upstreaming from https://phabricator.wikimedia.org/T218450 in case others run into this.
Any recommendations how to debug further are welcome.

Steps to reproduce:

Expected outcome:

Actual outcome:

Exception: Attempting to add more metadata after metadata has been locked.

More information:

Downstream version information:
phabricator: 05afa15ce649ee208cf3c022c75aa85ae390eabc (Jul 04 2019)
arcanist: f43c63ef5aaafaa8bf32ba4784d167a0448efd1a (Dec 20 2018)
phutil: c2d604923187a09b692d50e6bd381d3b91f5ef3a (Jul 04 2019)
php: 7.2.16-1+0~20190307202415.17+stretch~1.gbpa7be82+wmf1

Event Timeline

aklapper created this task.Jul 6 2019, 9:37 AM

@20after4, any chance there's a stack trace for this in the webserver error logs? I can't immediately figure out how to reproduce this locally.

The "Attempting to add more metadata after metadata has been locked." error means that we've supposedly finished rendering the page but some component is trying to add more metadata (usually, Javascript behaviors) to the page too late in the process, but it isn't very helpful on its own in pinpointing what the actual problem is. I'm not immediately sure how to reproduce this locally -- copying the description and image comment into a local mock didn't raise any errors.

I'm guessing it might be something like "use a certain specific remarkup rule in an inline comment" but I'm not having much luck figuring out what rule it might be.

We can probably make this error message more informative if we don't have any luck with the stack trace.


I did run into one not-likely-related-but-easily-reproducible-and-sort-of-adjacent error in poking around here:

  • In Pholio, leave an inline comment on an image, but don't provide any text, to produce this:

I toggled developer mode on for a moment and managed to get a stack trace:

25phabricatorinfrastructure/javelin/markup.php : 22CelerityStaticResourceResponse::addMetadata()
24phabricatorview/AphrontTagView.php : 161javelin_tag()
23phabricatorinfrastructure/markup/rule/PhabricatorObjectRemarkupRule.php : 177AphrontTagView::render()
22phabricatorinfrastructure/markup/rule/PhabricatorObjectRemarkupRule.php : 108PhabricatorObjectRemarkupRule::renderHovertag()
21phabricatorinfrastructure/markup/rule/PhabricatorObjectRemarkupRule.php : 84PhabricatorObjectRemarkupRule::renderObjectRef()
20phabricatorinfrastructure/markup/rule/PhabricatorObjectRemarkupRule.php : 389PhabricatorObjectRemarkupRule::renderObjectRefForAnyMedia()
19phutilmarkup/engine/PhutilRemarkupEngine.php : 293PhabricatorObjectRemarkupRule::didMarkupText()
18phabricatorinfrastructure/markup/PhabricatorMarkupEngine.php : 144PhutilRemarkupEngine::postprocessText()
17phabricatorinfrastructure/markup/PhabricatorMarkupEngine.php : 73PhabricatorMarkupEngine::process()
16phabricatorinfrastructure/markup/view/PHUIRemarkupView.php : 90PhabricatorMarkupEngine::renderOneObject()
15phabricatorview/AphrontView.php : 222PHUIRemarkupView::render()
14phutilmarkup/render.php : 111AphrontView::producePhutilSafeHTML()
13(Internal)phutil_escape_html()
12phutilmarkup/render.php : 181array_map()
11phabricatoraphront/response/AphrontResponse.php : 317hsprintf()
10(Internal)AphrontResponse::processValueForJSONEncoding()
9phabricatoraphront/response/AphrontResponse.php : 333array_walk_recursive()
8phabricatorapplications/celerity/CelerityStaticResourceResponse.php : 279AphrontResponse::encodeJSONForHTTPResponse()
7phabricatorview/page/PhabricatorStandardPageView.php : 583CelerityStaticResourceResponse::renderHTMLFooter()
6phabricatorview/page/AphrontPageView.php : 51PhabricatorStandardPageView::getTail()
5phabricatorview/page/PhabricatorStandardPageView.php : 889AphrontPageView::render()
4phabricatoraphront/configuration/AphrontApplicationConfiguration.php : 715PhabricatorStandardPageView::produceAphrontResponse()
3phabricatoraphront/configuration/AphrontApplicationConfiguration.php : 301AphrontApplicationConfiguration::produceResponse()
2phabricatoraphront/configuration/AphrontApplicationConfiguration.php : 209AphrontApplicationConfiguration::processRequest()
1/srv/deployment/phabricator/deployment-cache/revs/61f10999d8837a8c9dbeea12f67b2554daf057ab/phabricator/webroot/index.php : 35AphrontApplicationConfiguration::runHTTPRequest()

Could this be related to the custom footer markup? The code in question hasn't changed in a long time but I suspect maybe it's related to changes in the remarkup rendering code that's adding metadata where it didn't previously?

Thanks!

The renderHTMLFooter() method does a bunch of Javascript stuff and isn't actually related to the page footer, but PHUIRemarkupView and renderObjectRefForAnyMedia() were enough of a hint that think I have a reproduction case locally now -- let me figure out what I actually did and this will likely be fairly easy to fix.

epriestley renamed this task from Exception "Attempting to add more metadata after metadata has been locked" when trying to view a specific Pholio mockup to Javelin behaviors which pass PHUIViews to configuration parameters fatal the page in an abrupt way.Jul 11 2019, 9:41 PM
epriestley triaged this task as Wishlist priority.
epriestley edited projects, added Javelin; removed Celerity, Pholio.

D20647 should fix this. It isn't very satisfying as a general resolution.

The issue is that we have a Javelin behavior which passes a View as a configuration parameter, e.g.:

Javelin::initBehavior('quack', array('view' => $some_view_object));

Later, we render all registered behaviors in a single block, and lock metadata before rendering them. However, $some_view_object may wish to register metadata during rendering. The "fix" is to hsprintf(...) it earlier, which explicitly serializes it.

It would probably be reasonable to do this:

  • mark the behavior list as locked;
  • serialize the behavior list, working one behavior at a time, either doing try/catch for each behavior or passing keys into the stack as labels and doing try/catch for each value serialization;
  • mark the metadata list as locked;
  • serialize the metadata list -- working one block at a time is probably pointless since we can't really provide any useful context about a weird piece of metadata.

There's also no real reason we need to apply these locks at all; we could individually serialize the behavior and metadata lists repeatedly until both are empty, and allow metadata serialization to yield behaviors and behavior serialization to yield metadata, with some hard cap like "if components yield complimentary components more than 32 times, assume we're stuck in a loop with a goofy behavior and metadata block yielding one another".

Offhand, I can't really come up with a legitimate use case for having metadata serialization register a behavior, though.

Anyway, probably the kind of question that's ripe to be revisited when we're forced to mess with this stuff for Quicksand or something like that.