WeakRef usage (not always supported and not recommended)

[wolfbeast]

Description

JavaScript promise rejection: WeakRef is not defined. Open browser console to see more details.

I noticed Gitea's web interface started using WeakRef (noticed when posting a reply to a PR review conversation).
Please note that UXP doesn't implement WeakRef and its usage is not recommended unless you explicitly want to give hints to the JS GC for the release of (overly large) memory. Even so there is no guarantee that a JS engine will honour those hints, as they have often very complex internal machinery to deal with garbage collection. Letting content interfere with this process is not desirable; UXP doesn't intend to implement this as a result (potential can of worms also for security considerations as it might open UAFs and the like).

Please consider removing WeakRef usage from Gitea's web interface. There should not be a reason for using it to begin with. Let the engine do its own housekeeping. There is currently the risk of JS scripting breaking if it runs into WeakRef being undefined errors.

Screenshots

I didn't think of capturing the error box

Gitea Version

1.23.1

Can you reproduce the bug on the Gitea demo site?

Yes

Operating System

Windows 10 22H2

Browser Version

Pale Moon 33.5.1

[wxiaoguang]

grep -r WeakRef web_src shows that there is no WeakRef in Gitea's codebase.

Could you elaborate where it is used?

[wxiaoguang]

After searching the built assets, I found this, it is from github's text-expander https://github.com/github/text-expander-element, it uses https://github.com/iansan5653/dom-input-range/blob/main/src/input-style-clone-element.ts#L72

The related change is github/text-expander-element@180d221 (8 months ago), then text-expander started using input-style-clone which uses WeakRef.

---

I think it's impossible to remove text-expander, it is heavily used to show markdown suggestions. Do you think it's possible to polyfill the WeakRef?

[wolfbeast]

That use of WeakRef on an input doesn't even make much sense.
I'm not too familiar with writing polyfills so I don't know how easy or difficult it would be. It attaches a .deref() to the object passed into the constructor but doesn't interfere with it otherwise AFAIK.
You can polyfill this by simply using dummy functions for the API if you're married to that github module and can't make changes to it. As explained the spec is only providing hints for the garbage collector.

[wxiaoguang]

That use of WeakRef on an input doesn't even make much sense.

Yep, but that's from a dependency's dependency.

You can polyfill this by simply using dummy functions for the API if you're married to that github module and can't make changes to it. As explained the spec is only providing hints for the garbage collector.

I think you can add some polyfills to Pale Moon's engine, then all websites use WeakRef could benefit. For example:

• https://github.com/jaenster/weakref-pollyfill
• https://github.com/ungap/weakrefs

[wolfbeast]

I absolutely understand there's a broader solution possible, but you can't plug a web-based polyfill into an application's javascript engine like that. (if only! that would simplify a lot of things with all these convenience/sugar functions that get added to ES)
I have an open issue for implementing a stub into the engine but it's not straightforward.

[wolfbeast]

What would be the best way to report this to your dependency's dependency? because it really should not be used this way. Even the W3C TAG Design Principles group cautions strongly against them even existing and they should at most be used for extremely specific targeted situations and never make their way into generic dependencies or broadly-used libraries...
(see also https://repo.palemoon.org/MoonchildProductions/UXP/issues/1740 where this was analyzed when the question for implementation came up)

[TheFox0x7]

Probably raise an issue/PR there: https://github.com/iansan5653/dom-input-range

[wolfbeast]

Will do. Thanks

[wolfbeast]

Unfortunately there has been no response from the dom-input-range dev on the issue for 3 weeks. The dev doesn't seem to be active.
I would offer a PR but I do not work in nor know TS, and have no clue how to even test any changes I would be making :(

[wxiaoguang]

Unfortunately, this is a common phenomenon in many open source projects .......... and that's why I think it could be polyfilled (#33407 (comment)) to tolerate the WeakRef usage for more sites ......

[GiteaBot]

We close issues that need feedback from the author if there were no new comments for a month. 🍵

[wolfbeast]

Thank you bot for not understanding that this was an open issue and the last post was a comment, not a question.
This remains an issue. Unfortunately I can't re-open this.

[wxiaoguang]

Thank you bot for not understanding that this was an open issue and the last post was a comment, not a question. This remains an issue. Unfortunately I can't re-open this.

TBH I don't want to disappoint you, but actually it is not Gitea's problem ..... If it is only Gitea's problem, we could polyfill it in Gitea's code base (We have done so: #28441, #26575, #23592, etc and IMO it's better to avoid polyfills because many of them are still incomplete)

However, many sites including GitHub also use WeakRef (that JS library is also used by GitHub), so if you don't introduce a general WeakRef polyfill in the browser engine, many sites are still broken.

[wolfbeast]

I know it's a dependency of a dependency issue. But neither the dependency nor the dependency of the dependency repo maintainers are responding to any sort of attempted contact. If they would, it could be solved for anyone using that dom-range module. Ultimately that means it becomes a Gitea problem because you chose to rely on this (clearly unmaintained) dependency, which unnecessarily attempts to reach deep into the js machinery with WeakRef for no other reason than it "being a thing".
It's not like I didn't try to get the root issue fixed. I have.

I am well aware that it would be preferable to implement at least a stub WeakRef in the browser (which would be there just to satisfy use without fallback of WeakRef and not do anything otherwise) and that is on our to-do list but it is proving a lot more complicated than it should be because of the complexities of our inherited SpiderMonkey engine.
Of note also is that GitHub doesn't have this issue Gitea has (I have no trouble getting a proper pop-up on issue mention that works and is clickable, etc. which is broken because of WeakRef in Gitea) and Gitea has been the only web GUI so far I've encountered that breaks on it not being present. "many sites" is an overstatement.

[wxiaoguang]

OK, maybe GitHub also polyfills.

Could you try whether this PR work?

-> Polyfill WeakRef #34025

[wolfbeast]

Could you try whether this PR work?

I'd love to, but I'm neither familiar with your build process nor set up to do any golang testing. Is there a way I can slot this into a deployed Gitea instance to test?

[wxiaoguang]

Hmm, I can test it locally (and have tested it locally) and then after it gets approved, then there will be a nightly built.

[wolfbeast]

OK let me know when you have a nightly build that I can drop into production to test, and I'll verify. Thanks!

[silverwind]

According to jaenster/weakref-pollyfill#1 WeakRef is not polyfillable, so while our pseudo-polyfill might works, it'll probably never act like the real thing, because JS does not expose the necessary garbage collector interfaces for it to be polyfillable.

[wxiaoguang]

According to jaenster/weakref-pollyfill#1 WeakRef is not polyfillable, so while our pseudo-polyfill might works, it'll probably never act like the real thing.

It won't be a problem, in our case, no memory management requirement, and actually the package using WeakRef seems abusing it (according to the discussions above).

[wolfbeast]

According to jaenster/weakref-pollyfill#1 WeakRef is not polyfillable

The front-end most definitely is, and that is all that matters. You shouldn't expect a polyfill to take over what is handled by GC internals, but that is the whole point of why it shouldn't actually be a thing to begin with. JS engines will always know much better when and how to collect garbage than any cross-browser, web-based content would. WeakRef makes no guarantees, and there are critical notes from everyone involved it shouldn't be relied on for memory management. It only provides hints that a script "would like something to be GCed", but whether it, in fact, is, depends entirely on the state of the JS engine internals. It may happen later or not at all. Providing a stub is therefore just fine.

[wxiaoguang]

1.2 nightly is ready

• https://dl.gitea.com/gitea/1.23-nightly/
• https://hub.docker.com/r/gitea/gitea/tags?name=1.23-nightly

[wolfbeast]

Sorry for the delay in answering - I can confirm the nightly build solves the issue and that the polyfill works as it should.

[wxiaoguang]

Glad to help.

ps: I can't access repo.palemoon.org, it always redirects to https://repo.palemoon.org/assets/geoblock.html

I guess this PR might help you: Add a config option to block "expensive" pages for anonymous users #34024 (background: #33966): when set REQUIRE_SIGNIN_VIEW=expensive, many pages will require sign-in to block AI crawlers.

[wolfbeast]

Yeah I was forced to blanket geoblock certain regions because of IP-spread hammering of our repo with crawling requests going through all tag combinations (requesting the same pages over and over and over...) causing 1500% CPU load which our host wasn't happy about ;P
I'll try the option and see how that goes. Thanks for the tip!

[wolfbeast]

REQUIRE_SIGNIN_VIEW=expensive

Unfortunately this won't work for us. requiring an account+signin to even view commits just isn't good practice for FOSS.
Also, seems you should really add "/compare" to it; they are some of the most expensive calls in gitea.

[wxiaoguang]

REQUIRE_SIGNIN_VIEW=expensive

Unfortunately this won't work for us. requiring an account+signin to even view commits just isn't good practice for FOSS. Also, seems you should really add "/compare" to it; they are some of the most expensive calls in gitea.

Then would something like Proof-of-Work works? For example: redirect anonymous users to a JS page, calculate something on user side and let server check.

[silverwind]

All public gitea instances deal with such problems. We already implement captcha for sign ups, maybe it's time to go further and add an option to require captcha for anonymous viewing.

Only downside is that that would likely also catch potential legitimate traffic like search crawlers.

[wolfbeast]

Interesting to brainstorm about, probably good to open a new issue for.