Page MenuHomePhabricator
Differential D15535

Fully fix a bad rule object aliasing issue custom remarkup rules
ClosedPublic
Actions

Authored by epriestley on Mar 28 2016, 5:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 7:28 PM
Unknown Object (File)
Fri, Dec 13, 7:10 PM
Unknown Object (File)
Thu, Dec 12, 7:11 AM
Unknown Object (File)
Wed, Nov 27, 12:54 PM
Unknown Object (File)
Sat, Nov 23, 6:42 AM
Unknown Object (File)
Nov 19 2024, 12:52 AM
Unknown Object (File)
Nov 15 2024, 3:13 PM
Unknown Object (File)
Nov 14 2024, 7:20 AM
View All Files
Subscribers
eadler
thoughtpolice

Details

Reviewers
thoughtpolice
chad
Maniphest Tasks
T10234: When using a PhabricatorRemarkupCustomInlineRule with caching disabled, custom rules may end up with incorrect engines
Commits
rPbf3879b1c72f: Fully fix a bad rule object aliasing issue custom remarkup rules
Summary

Fixes T10234. This is a more thorough fix.

Root issue is that some time around D13589, we started hitting an object cache for loadCustomInlineRules(), but didn't adjust the code to account for that.

So if a page created multiple similar engines, we'd return the same $rule object for multiple engines, call setEngine() on it with different engines, and then possibly try to render using an already-expired engine the second time through.

Instead, create a separate $rule object for each separate $engine.

Test Plan

Repro is something like this:

  • Create a custominlinerule which uses an engine.
  • Purge the remarkup cache.
  • Load a page which uses the rule in two engines (e.g., in a revision description, and also in an inline comment).
  • Before change: second one could fatal. After change: clean load.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 37451.Mar 28 2016, 5:24 PM
epriestley retitled this revision from to Fully fix a bad rule object aliasing issue custom remarkup rules.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
epriestley added a task: T10234: When using a PhabricatorRemarkupCustomInlineRule with caching disabled, custom rules may end up with incorrect engines.
Herald edited edge metadata. · View Herald TranscriptMar 28 2016, 5:24 PM
thoughtpolice accepted this revision.Mar 28 2016, 5:32 PM
thoughtpolice added a reviewer: thoughtpolice.
thoughtpolice added subscribers: eadler, thoughtpolice.

I've confirmed this fixed our instance of T10234 on phabricator.haskell.org

(/cc @eadler, who may want to note this for Wikimedia?)

epriestley mentioned this in T10234: When using a PhabricatorRemarkupCustomInlineRule with caching disabled, custom rules may end up with incorrect engines.Mar 28 2016, 5:36 PM
eadler added a comment.Mar 28 2016, 5:43 PM
In D15535#179416, @thoughtpolice wrote:

I've confirmed this fixed our instance of T10234 on phabricator.haskell.org

(/cc @eadler, who may want to note this for Wikimedia?)

I deal with FreeBSD, my employers, and a few other small instances, but appreciate the CC nonetheless.

chad accepted this revision.Mar 28 2016, 6:20 PM
chad edited edge metadata.

¯\_(ツ)_/¯

This revision is now accepted and ready to land.Mar 28 2016, 6:20 PM
Closed by commit rPbf3879b1c72f: Fully fix a bad rule object aliasing issue custom remarkup rules (authored by epriestley, committed by epriestley). · Explain WhyMar 28 2016, 6:27 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
infrastructure/
markup/
2 lines

Diff 37458

View Options

src/infrastructure/markup/PhabricatorMarkupEngine.php

Loading...
Phabricator · Built by Phacility