Skip to content

ObjectURL api#548

Merged
saghul merged 2 commits intosaghul:masterfrom
EmixamPP:feat/objecturl
Jul 9, 2024
Merged

ObjectURL api#548
saghul merged 2 commits intosaghul:masterfrom
EmixamPP:feat/objecturl

Conversation

@EmixamPP
Copy link
Copy Markdown
Contributor

@EmixamPP EmixamPP commented Jun 17, 2024

ObjectURL API doc: https://developer.mozilla.org/en-US/docs/Web/API/URL/createObjectURL_static

It is more complicated than expected to solve #479.

I wanted to use the JS Blob.text() method to retrieve the content of the stored blob (as a JSValue) in a global blob store. But the function is async, and I'm not sure the JS_Call method can resolve and wait such a function, since it raises tjs__promise_rejection_tracker: Assertion '(JS_IsException(event)) == (0)' failed..

Anyway, here is a draft PR that shows what I've tried so far, maybe you will be able to help me. Or my solution is too bad (I have the feeling that it is maybe a bit too hacky?), and it will be easier for you to start from scratch when you will have time later (don't worry if this is the case).

While I'm waiting for your review, I'll keep trying to fix and improve the solution.

@EmixamPP EmixamPP marked this pull request as draft June 20, 2024 06:44
@EmixamPP EmixamPP changed the title draft: ObjectURL api ObjectURL api Jun 20, 2024
@saghul
Copy link
Copy Markdown
Owner

saghul commented Jun 20, 2024

Good start!

What I had in mind was perhaps a bit simpler:

  • Use the builtin SQLite module to store the Blob (probably in parts?) in an in-memory table
  • Use crypto.randomUUID() for the identifier

Then when creating the worker you can grab all the Blob parts straight from the SQLite DB and re-construct the content. SInce the sqlite module is sync there is no need to await anyhing.

@EmixamPP EmixamPP force-pushed the feat/objecturl branch 2 times, most recently from 779101b to b14f873 Compare June 20, 2024 15:19
@EmixamPP
Copy link
Copy Markdown
Contributor Author

Sorry, I had to push force because I authored then committed with a wrong email.

So I got the api almost working in every case. Actually, in the test, I've added the revoke of the url after creating the worker. However, if you execute it a lot, sometimes, it will be done a bit too soon; before being queried by the eval. I will fix that.

I've used the crypto.randomUUID() for the url generation directly at the js side. And I will investigate then to use the sql module instead of using a global hash map as blob store.

I've made the new commits as fixup for their previous related commit, like this when the review is done, I can squash them automatically to keep a clean commit history, while being able to review them easily.

@saghul
Copy link
Copy Markdown
Owner

saghul commented Jun 20, 2024

Feel free to keep pushing changes, we can squash when merging.

@EmixamPP
Copy link
Copy Markdown
Contributor Author

I have the feeling that going for an in-memory sqlite database may be overkill, so here I simply use a js Map(). Also, mainly because, after a re-read of the api here: https://developer.mozilla.org/en-US/docs/Web/API/URL/createObjectURL_static. I read that the liftime of the url is linked to the scope of the document (in the browser), so globally scoped across runtime object is unnecessary. Let me know what you think about that.

I got a working implementation. Now, maybe some work on the style and structure are needed?
E.g. I made the use of a magic string between url.js and objecturl.c to know the name of the storage map. So, it may be better to create the storage map from C during the loading of the core module, but then, even if the API is not used, an empty map will be created.

I wanted to use the sessionStorage but that one only store string, so that is a problem here, since the string value cannot be retrieved synchronously in js.

@EmixamPP EmixamPP requested a review from saghul June 26, 2024 11:26
@EmixamPP
Copy link
Copy Markdown
Contributor Author

Rebase on master.

}
});

// TODO: straggly, without the wrapped console.log, the runtime will abort
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that is very weird. Do you have a backtrace?

Copy link
Copy Markdown
Contributor Author

@EmixamPP EmixamPP Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is different now, the function is spammed. I made a small video capture to show you:
objecturl_bug.webm
Therefore, with the current case, I have no backtrace to provide. And I have no clue what is that printed SyntaxError, since there is no problem at all when running it with the empty console.log('').

@saghul
Copy link
Copy Markdown
Owner

saghul commented Jun 27, 2024

Thank you! I did a full review round and left some comments.

@EmixamPP EmixamPP marked this pull request as ready for review June 28, 2024 14:19
@EmixamPP EmixamPP requested a review from saghul June 28, 2024 14:20
@EmixamPP EmixamPP requested a review from saghul July 2, 2024 06:42
@EmixamPP EmixamPP requested a review from saghul July 2, 2024 10:46
It doesn't really work and causes runtime startup problems.
@saghul
Copy link
Copy Markdown
Owner

saghul commented Jul 9, 2024

@EmixamPP I discovered the source of the problem! The fflate package which we use as part of the compression streams polyfill was using the Worker API while the engine was not fully ready, in order to detect support.

I disabled it since it didn't actually work, and squashed and force-pushed!

@saghul saghul merged commit a5503da into saghul:master Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants