Skip to content

Migrate to manifest v3 - #214

Open
jupenur wants to merge 7 commits into
mainfrom
mv3
Open

Migrate to manifest v3#214
jupenur wants to merge 7 commits into
mainfrom
mv3

Conversation

@jupenur

@jupenur jupenur commented Jul 28, 2026

Copy link
Copy Markdown
Member

Resolves #58.

@jupenur
jupenur requested a review from lsd-cat July 28, 2026 15:57
@jupenur

jupenur commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

This is targeting the #213 branch, that's why you don't see any tests running here. Review and merge #213 first and GitHub will automatically retarget this to main, then the tests should run.

Base automatically changed from persistence to main July 29, 2026 06:57
@lsd-cat
lsd-cat requested a review from a team July 29, 2026 06:57
@jupenur

jupenur commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

Rebased

@lsd-cat

lsd-cat commented Jul 29, 2026

Copy link
Copy Markdown
Member

I wonder what's your opinion on keeping the cache in or our the persistent storage? I don't know how often the bg page will actually be terminated, but my impression would be that the cache would lose a lot of utility this way

@jupenur

jupenur commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

The default timeout is only 30 seconds, so I agree we should probably persist the origin cache. Looking into it now.

@jupenur

jupenur commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

OriginState objects are now persisted per session, matching the pre-MV3 behavior. I decided not to explicitly persist nonOrigins because the in-memory set is already populated via a simple storage lookup here:

// 3. Storage lookup
const stored = await this.enrollments.get(fqdn);
if (stored) {
return new Uint8Array(stored);
} else {
this.nonOrigins.add(CacheKey(fqdn, cachePartition));
return new Uint8Array();
}

@jupenur

jupenur commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

There was some refactoring involved in making the OriginState classes compatible with session storage. Namely, OriginStateHolder is now gone, and there's only one mutable OriginState class instead; the LRUCache API is now asynchronous (and for consistency so is LRUSet) and subclassed into PersistentLRUCache; and the OriginState class implements a toPOJO method that returns a plain JavaScript object that's compatible with the storage API.

@lsd-cat

lsd-cat commented Aug 10, 2026

Copy link
Copy Markdown
Member

This is great, thank you! I've spent some time trying to figure out the implications of using the storage, especially for risks of race conditions (not necessarily security-sensitive, but still). I'm gonna take some more to brainstorm and test more to see if I can spot and test concrete cases

@jupenur

jupenur commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

03364e7 should resolve some of the concerns around race conditions.

@lsd-cat lsd-cat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, it's great to see this live! I think it's well structured and will improve mantainability, as it seems already with the lib draft PR. The comments are only about worrying whether adding awaits to code we expect to be sequential actually breaks some guarantees we expected in the past. I'm gonna do some more testing a dig a bit more into real behavior to see if they are plausible, but they are not necessarily blocking. Thanks also for making the cache atomic.

}

#commitVerifiedOrigin(
async #commitVerifiedOrigin(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I got a bit in a rabbit hole here. It is my understanding that executions are normally sequential and the extension backend is single thread, so inside the same function we should have some degree of atomicity. However, I'm unsure what the behavior is when we await and if execution is potentially passed to other contexts and then resumed when the await has completed. If that's the case, there's a a potential race here and in similar code patterns. Though it's pretty unlikely, I'd just like to double check if the reasoning holds

batch[hostname] = Array.from(rawHash);
}

await this.enrollments.clear();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not specific to this PR, but probably an earlier regression of when we moved out of IndexeDb. If the await/async assumption is correct, then there's a tiny window where the list is empty and an enrolled website could be skipped.

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.

Manifest v3

2 participants