The media filter allows infinite recursion via media_token_to_markup()

This looks good to me. The infinite recursion bug only happens when the text format of the self referencing text field is cacheable, because only then is the text processed during the load operation (via text_field_load()). I wonder if the code comment should include this info.

Also, currently the filter never renders the file's fields, but at some point, as part of another issue, it will need to, in which case, this fix won't be sufficient when the text format isn't cacheable.

Upping to major, because although infrequent, it causes a fatal error when it does happen.

This needed a reroll, also setting as 1.x as I'm pretty sure that's the branch this was originally intended for.

We've encountered other problems before because the media filter is cached. Those problems seem to be resolved by marking the filter as uncacheable. Does that seem like something we should consider more seriously since it would resolve this issue?

If this happens on the 2.x branch, we should fix it there first and then backport it.

#4: 1266064-media-infinite-recursion-4.patch queued for re-testing.

Related to #4: #1984424: RFC: Disable caching for media filter

I strongly disagree with turning off caching of the media filter. I have run into several scenarios in the past 6 months where a 'cache' => FALSE setting in hook_filter_info() has brought a production site to its knees, causing all sorts of unexpected slowness and out of memory errors. To quote the hook_filter_info documentation:

cache (default TRUE): Specifies whether the filtered text can be cached. Note that setting this to FALSE makes the entire text format not cacheable, which may have an impact on the site's overall performance.

For us, adding 'cache' => FALSE did work and resolved the WSODs we were getting on pages with embedded media, BUT the proposed patch above is a much more favorable solution. That said, I believe there is a major flaw in the current patch:
unset($current_fids[$tag_info['fid']]);

By unsetting the value that was previously set, it creates as much of a potential for recursion as before, rendering the static caching useless, right? Local testing verified that with the patch as written (offset to a different line number in the latest 7.x-2.x-dev release) was still causing out of memory errors for us and the issue was not resolved.

I have re-rolled the patch for the latest 7.x-2.x-dev, consolidated some of the code, and removed the unset() call altogether. Our filters are fully cached and we're no longer experiencing the WSODs associated with infinite recursion/out-of-memory/timeout errors.

I'm also upgrading this issue to critical because this issue has the potential to break overall site functionality.

I just noticed that the patch in #10 will throw exceptions when catching recursion and throw php warnings of "undefined variable 'file'"...

Updated to correct those errors.

Caught another small issue -- returning '' can prevent images from rendering when recursion is caught. Updated against latest HEAD and added logging instead.

Chasing HEAD. Re-rolled.

Note as of this commit http://drupalcode.org/project/media.git/commit/7f322d9 you will need to enable the new 'Media Wysiwyg' submodule.

The patch proposed in #4 became much clearer to me this morning regarding the unsetting of the $current_fids value. The previous handful of patches were attempting to address an efficiency issue, however they backfired and prevented images from loading under different circumstances. Please disregard my comment in #10 about the static caching method employed in the patch above; I misunderstood the approach taken to catch the recursion.

This latest re-roll is an update from the original patch in #4 with a little more information thrown in the exception(s) and a developer's note regarding the unsetting of the safety catch.

14: media-infinite-recursion-1266064-14.patch queued for re-testing.

+++ b/modules/media_wysiwyg/includes/media_wysiwyg.filter.inc
@@ -103,11 +103,26 @@ function media_wysiwyg_token_to_markup($match, $wysiwyg = FALSE) {
+    ¶

Besides this whitespace i think this scenario could use a testcase. Also do we really need to throw an exception? Maybe just watchdog it? nvm it is already in a try/catch block that watchdogs the errorrs

Are you finding that the statics work correctly after cache clear?

I'm getting things working as you'd expect on a normal page load, but when the cache is cleared recursion is detected (even when there is no recursion) and the filter is not doing it's replacements.

Reloading the page after cache clear will show all the filter replacements happening as usual. Clear the cache again and they disappear. Reload, reappear. You get the picture.

Is there something silly about the some cache that breaks the way a PHP static works? Should we be using a drupal_static instead, or would that not help here?

update: it was the field cache that was causing the static issues. After running drush sqlq "truncate table cache_field;" the static was triggered accidentally. reload the page, and it's not.

I'm still baffled as to why the static value in the filter system would be different after a field_cache clear than if the field was pulled from cache.

Okay, I've tracked my problem down to one line of code in my filter:

node_load(arg(1));

It looks like if you load a node while filtering (replacing a string) in a field on that same node, then the static gets set inadvertently.

...still not sure how to fix this or work around it, but I think I'm getting closer.

interesting... by enabling the entity cache module the problem no longer appears on the first page load after running drush sqlq "truncate table cache_field;" However, it is still present when clearing the "Page and else" cache via admin_menu. Will track down which table it is...

Update: of course. instead of cache_field the table that's now causing the issues is cache_entity_node. Not so interesting after all!

Aha! The things you find in your Drupal... I have outlined a detailed description of the problem I ran into over here #2470503: Can't load (same) entity while filters are being run on any field on that entity

I'm not sure if this is directly related to media, but I have a feeling it may be.

Here's a fix that solves the recursive entity load problem for filters.

sorry, not critical. Not sure how that happened.

Oh, this is the wrong issue completely! Please ignore that patch :) (restoring critical)

Closing this issue as outdated. However, if you think this issue is still important, please let us know and we will gladly re-open it for review.
sincerely,
- the Drupal Media Team