Service tester updates#24
Conversation
improved build tester scoring
- scripts/_mixin.py: switch moz_target from x86_64-pc-mingw32 (no longer supported in FF150) to x86_64-pc-windows-msvc - additions/juggler/screencast/HeadlessWindowCapturer.h: typedef pid_t on XP_WIN; libwebrtc headers (video_capture.h, desktop_capturer.h) reference pid_t which is POSIX-only - patches/anti-font-fingerprinting.patch: include mozilla/dom/Document.h in gfxTextRun.cpp; on Windows it is not transitively included so doc->GetInnerWindow() failed with "incomplete type 'Document'" Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR upgrades the Camoufox Firefox fork from version 146.0.1 to 150.0.2 while introducing six per-user-context fingerprinting managers, real preset support by Firefox version, a complete Juggler target registry, and refactored test infrastructure using local executables. ChangesFirefox 150 upgrade and build infrastructure
Per-user-context spoofing and fingerprinting framework
Juggler target registry and page/browser control
Real fingerprint presets and test infrastructure refactoring
Configuration, branding, and masking integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
patches/locale-spoofing.patch (2)
55-70:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't return
c_str()from temporary MaskConfig strings.
lang.value(),region.value(), andscript.value()are destroyed at the end of eachif, so these helpers return dangling pointers.Language(),Script(), andRegion()then immediately read invalid memory viastrlen(...)/Set(...). Please return an owning string type here or cache the MaskConfig values in storage with a stable lifetime before exposing raw pointers.Also applies to: 122-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/locale-spoofing.patch` around lines 55 - 70, Locale::GetCamouLanguage, GetCamouRegion, and GetCamouScript currently return pointers to temporaries (lang.value().c_str()) which dangle; change them to return an owning type or provide a stable-cached storage: either return std::string (or std::optional<std::string>) instead of const char*, or store the MaskConfig values in persistent members (e.g., Locale::camouLanguage, camouRegion, camouScript) that are populated from MaskConfig::GetString and return pointers to those members; update callers Language(), Script(), and Region() to accept the new return type or use the cached members so strlen/Set read valid memory.
9-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
locale:allnever reaches the C++ locale path.Startup writes
intl.accept_languagesfromlocale:all, butLocale::GetCamouLocale()only consultslocale:language,locale:region, andnavigator.language. If the caller provides onlylocale:all,Locale::GetDefaultLocale()andOSPreferencesstill fall back to the real system locale, which reintroduces a locale fingerprint mismatch. Please normalizelocale:allinGetCamouLocale()too, or centralize the locale selection logic so both paths consume the same value.Also applies to: 75-93, 172-197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/locale-spoofing.patch` around lines 9 - 23, The C++ path (Locale::GetCamouLocale / Locale::GetDefaultLocale / OSPreferences) doesn't consult the JS-only "locale:all" value, so normalize or centralize the selection: update Locale::GetCamouLocale to check for the "locale:all" camouflaged string (and parse its comma-separated list into the same canonical "language[-region], language" form the JS code produces) or refactor so both JS (ChromeUtils.camouGetString usage) and C++ share a single helper that resolves "locale:all", "locale:language", "locale:region", and "navigator.language"; ensure the resulting string is what Services.prefs.setCharPref("intl.accept_languages", ...) would set so both startup and OSPreferences consume the identical locale value.patches/geolocation-spoofing.patch (1)
17-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the geolocation permission gate even for spoofed coordinates.
This short-circuits to
AllowbeforeCheckPromptPrefs(), so any site gets geolocation access whenever spoofed lat/lon are configured, even if the site would normally prompt or has already been denied. The spoof should replace the coordinates, not bypass the permission decision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/geolocation-spoofing.patch` around lines 17 - 22, The added block short-circuits permission handling by calling request->RequestDelayedTask(nsGeolocationRequest::DelayedTaskType::Allow) and returning true when MaskConfig::GetDouble("geolocation:latitude") && MaskConfig::GetDouble("geolocation:longitude") are present; instead, remove the early Allow/return and only record the spoofed latitude/longitude (read via MaskConfig::GetDouble) on the request or a small request-local field so the normal permission path (CheckPromptPrefs(), existing prompt/deny logic) still runs; after the permission is granted by the existing flow, inject or replace the coordinates on the response using the stored spoof values (refer to nsGeolocationRequest and CheckPromptPrefs() to locate where to apply the spoofed coords).patches/font-hijacker.patch (1)
50-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't replace the font loading state machine with the allowlist result.
FontFace::Status()now reports every allowed face asLoaded,FontFace::Load()no longer callsmImpl->Load(aRv), andFontFaceImpl::SetStatus()ignores the actual loader status. That breaks@font-facebehavior: allowed fonts can resolve before any resource fetch completes, and loading/failed states are no longer observable. Keep the existing status transitions intact and apply the allowlist at the family-resolution/gating layer instead.Also applies to: 88-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/font-hijacker.patch` around lines 50 - 80, Revert the logic that substitutes the loader state with the allowlist: restore FontFace::Status() to return mImpl->Status() and make FontFace::Load() call mImpl->Load(aRv) (keeping mImpl->UpdateOwnerKeepAlive()), rather than resolving/rejecting immediately based on GetFamily()/mozilla::dom::IsFontAllowed(). Instead, implement the allowlist check at the family-resolution/gating layer (where the network/resource fetch or loader completion is observed) so you only gate final promise resolution/rejection there—leave FontFaceImpl::SetStatus() to drive the normal Loaded/Loading/Error transitions and only consult IsFontAllowed when deciding whether to allow resolution after the loader reports success or to convert a successful load into a blocked/failure outcome.patches/librewolf/mozilla_dirs.patch (1)
5-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the new
XREMoz*NativeManifestslookups on unsupported platforms.
NativeManifests.sys.mjsnow resolves these two directory-service properties unconditionally, but the provider code in this patch only handles them inside theXP_UNIX || XP_MACOSXbranch. On Windows,Services.dirsvc.get(...)will throw before the native-manifest fallback can run. Please skip these entries on unsupported platforms or handle missing properties defensively.Also applies to: 50-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/librewolf/mozilla_dirs.patch` around lines 5 - 10, The new unconditional calls to Services.dirsvc.get for XREMozUserNativeManifests and XREMozSysNativeManifests in NativeManifests (NativeManifests.sys.mjs) cause throws on unsupported platforms; update the code that builds dirs (where Services.dirsvc.get("XREMozUserNativeManifests", ...) and Services.dirsvc.get("XREMozSysNativeManifests", ... ) are invoked) to only attempt those lookups when the platform check matches (the same XP_UNIX || XP_MACOSX branch used by the provider) or wrap each lookup in a defensive try/catch that skips missing properties, so Windows won't call Services.dirsvc.get for those keys and the native-manifest fallback can run.additions/juggler/TargetRegistry.js.bak (1)
1-1307:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDelete the
TargetRegistry.js.bakfile entirely.This file is dead code. Firefox loads only
chrome://juggler/content/TargetRegistry.js(the non-.bakversion); nothing in the codebase imports or uses the.bakvariant. Worse, the.bakfile is an older intermediate checkpoint missing critical fixes: it contains seven[TR-DEBUG]debug console.error statements (lines 323, 357, 359, 361, 366, 370, 373) that were cleaned from the production version, and it lacks a 14-line Firefox 150+ workaround for gBrowser population that exists in the main file. Shipping both creates silent drift and maintenance confusion. Remove it in the same commit that adds or modifies the actualTargetRegistry.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@additions/juggler/TargetRegistry.js.bak` around lines 1 - 1307, Remove the dead backup file TargetRegistry.js.bak entirely; it contains stale code (including console.error “[TR-DEBUG]” statements in _newPageInternal) and diverges from the canonical TargetRegistry implementation (see TargetRegistry class, PageTarget, and waitForWindowReady), so delete this file and include that deletion in the same commit that adds/modifies the real TargetRegistry.js to avoid drift and confusion.patches/timezone-spoofing.patch (3)
27-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
GetTimezonehas a write side effect — surprising for a getter.When the per-context key is missing, the MaskConfig fallback branch writes the value back into
RoverfoxStorageManagerbefore returning. That:
- promotes a read into a cross-process IPC write (
SendRoverfoxStoragePuton the parent path), which is expensive on a hot path;- changes observable persistent state from what looks like a pure query;
- can race with concurrent
SetTimezonecalls on other threads/contexts.Either rename to make the mutation explicit, or just return the MaskConfig value without persisting it (let the next
SetTimezonecall own the write).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/timezone-spoofing.patch` around lines 27 - 44, TimezoneManager::GetTimezone currently writes to RoverfoxStorageManager when falling back to MaskConfig, which is a surprising mutation for a getter; change the fallback branch in TimezoneManager::GetTimezone to avoid the write side effect by removing the RoverfoxStorageManager::PutString call and simply convert MaskConfig::GetString("timezone") into outTimezone (NS_ConvertASCIItoUTF16) and return true, leaving persistence to SetTimezone (or alternatively rename the method if you intentionally want it to persist, but do not perform the PutString inside this getter).
148-188:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPer-context timezone setter has two cross-context isolation breaks.
Two distinct problems in
SetTimezone:
SetTimezone(0, timezone)poisons context 0.TimezoneManager::SetTimezonealways callsDisableFunction(userContextId)(line 24). So when a window in context N ≠ 0 callssetTimezone, you also writetimezone_0and settz_disabled_0 = true, which permanently hidessetTimezonefrom every context-0 window even though context 0 never called it. Combined withGetTimezonenot falling back fromtimezone_<N>totimezone_0, the fallback write to 0 has no read-side benefit and only causes the lockout.
JS::SetTimeZoneOverride(timezoneASCII.get())is process-wide. This mutates a global SpiderMonkey state shared by every realm in the process. The ai-summary describes this as “ultimate fallback” for shared/service workers, but per the patch's design every worker realm already gets a per-realm override viaGetOrCreateGlobalScope(WorkerPrivate.cppchanges) andgetDateTimeInfo()readsbehaviors_.timeZoneOverride(). The last context to callsetTimezonetherefore wins globally, breaking the per-context promise the rest of the patch is built on.Recommend dropping both the
SetTimezone(0, ...)block and theJS::SetTimeZoneOverride(...)call, and instead extending the fallback read path inTimezoneManager::GetTimezone(or in worker startup) to read per-realm storage only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/timezone-spoofing.patch` around lines 148 - 188, The SetTimezone implementation introduces two cross-context breaks: remove the fallback write to context 0 and the process-wide SpiderMonkey override; specifically, delete the branch that calls TimezoneManager::SetTimezone(0, timezone) (the block guarded by if (userContextId != 0)) and remove the JS::SetTimeZoneOverride(timezoneASCII.get()) call, and instead update TimezoneManager::GetTimezone (or the worker startup read path used by WorkerPrivate/getDateTimeInfo) to consult per-realm storage and perform the intended fallback there so shared/service workers can read an appropriate per-realm value without poisoning userContextId 0 or mutating global SpiderMonkey state.
368-383:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCache
DateTimeInfoand invalidate only on timezone override changes.
getDateTimeInfo()is called for everyDateconstruction, locale-sensitive operation, andIntl.DateTimeFormatoperation. The current implementation unconditionally allocates a newDateTimeInfowithjs::MakeUnique<js::DateTimeInfo>(tzOverride->chars())on every call. Instead, cache the constructedDateTimeInfoand only rebuild it whenbehaviors_.timeZoneOverride()actually changes. Timezone overrides are set infrequently (viasetTimeZoneOverride, document navigation, or worker setup)—the "always create fresh" approach incurs unnecessary heap allocations on a genuinely hot code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/timezone-spoofing.patch` around lines 368 - 383, Realm::getDateTimeInfo currently recreates a js::DateTimeInfo on every call; change it to reuse the cached dateTimeInfo_ and only rebuild when behaviors_.timeZoneOverride() actually changes. In getDateTimeInfo(), fetch RefPtr<JS::TimeZoneString> tzOverride = behaviors_.timeZoneOverride(), compare tzOverride->chars() (or the RefPtr identity) against the last-used value (store the last timezone string or the RefPtr in the Realm instance), and only call js::MakeUnique<js::DateTimeInfo>(...) to replace dateTimeInfo_ when the timezone differs; otherwise return the existing dateTimeInfo_.get(); ensure this logic is inside the JS_HAS_INTL_API block and preserves existing behavior when tzOverride is null (return nullptr).patches/anti-font-fingerprinting.patch (3)
1324-1342:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCase-transformed text runs always get
userContextId = 0.
nsCaseTransformTextRunFactory::RebuildTextRunhardcodesprivateBrowsingId = 0, so any text passing through CSStext-transform: uppercase/lowercase/capitalize(and inner factories that wrap it) will bypass per-context font spacing perturbation entirely, leaving a fingerprintable hole that distinguishes transformed vs. untransformed runs.Consider plumbing
aUserContextIdthroughnsTransformingTextRunFactory::MakeTextRun(or via the parent text-run/gfxFontGroup::mUserContextId) so the transformed children inherit the right ID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/anti-font-fingerprinting.patch` around lines 1324 - 1342, The code hardcodes privateBrowsingId = 0 in nsCaseTransformTextRunFactory::RebuildTextRun causing transformed text runs to lose the real user/context id; modify the plumbing so the proper userContextId is propagated — either add a parameter aUserContextId to nsTransformingTextRunFactory::MakeTextRun (and update callers like mInnerTransformingTextRunFactory->MakeTextRun) or read and forward gfxFontGroup::mUserContextId when constructing child runs; ensure nsCaseTransformTextRunFactory::RebuildTextRun passes that real id into the fontGroup->MakeTextRun/inner MakeTextRun calls so transformed children inherit the correct context instead of 0.
97-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing newline at end of
FontSpacingSeedManager.cpp.The patch ends without a trailing newline (
\ No newline at end of file). Add one to satisfy POSIX text-file conventions and avoid noisy diffs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/anti-font-fingerprinting.patch` around lines 97 - 98, The file ends with the line "} // namespace mozilla" but lacks a trailing newline; open FontSpacingSeedManager.cpp, add a single POSIX newline character at EOF so the file ends with a blank line after the closing " } // namespace mozilla" to satisfy text-file conventions and remove the "\ No newline at end of file" marker.
864-896:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winLetter-spacing perturbation compounds glyph drift (double-offset).
In
gfxHarfBuzzShaper::ShapeText, every glyph receives both:
x_advance += spacing(moves the pen by an extraspacingper glyph), andx_offset += cumulativeOffset(renders glyphishifted byi * spacing).Combined with the advance accumulation, the visual position of glyph
iends up offset by roughly2 * i * spacingfrom the unspoofed layout, while clusters/pen positions only advance byi * spacing. This makes glyphs drift further than the pen tracks, which can cause overlap with surrounding runs, hit-testing/selection mismatches, and stronger-than-intended visual perturbation. The same applies in the vertical branch.If the intent is normal letter-spacing perturbation, drop the
*_offsetmutation and only adjust*_advance. If the cumulative offset is deliberate, please document the rationale and verify downstream selection/measurement code tolerates the resulting advance/offset mismatch.🐛 Proposed fix: spacing via advance only
hb_position_t spacing = FloatToFixed(randomFloat); uint32_t glyphCount; hb_glyph_position_t* glyphPositions = hb_buffer_get_glyph_positions(mBuffer, &glyphCount); - hb_position_t cumulativeOffset = 0; - // Apply custom letter spacing for (uint32_t i = 0; i < glyphCount; ++i) { if (aVertical) { glyphPositions[i].y_advance -= spacing; - glyphPositions[i].y_offset -= cumulativeOffset; } else { glyphPositions[i].x_advance += spacing; - glyphPositions[i].x_offset += cumulativeOffset; } - cumulativeOffset += spacing; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/anti-font-fingerprinting.patch` around lines 864 - 896, The letter-spacing perturbation doubles per-glyph displacement because ShapeText in gfxHarfBuzzShaper modifies both advances and offsets (glyphPositions[i].x_advance/x_offset and y_advance/y_offset) and also accumulates cumulativeOffset; to fix, remove the offset mutations and the cumulativeOffset accumulation and only adjust x_advance (or y_advance when aVertical) by spacing (i.e., drop changes to x_offset and y_offset and the cumulativeOffset logic) so the perturbation is applied via advances alone; update the code around glyphPositions, spacing, and the aVertical branch in gfxHarfBuzzShaper::ShapeText accordingly.patches/audio-fingerprint-manager.patch (2)
246-345:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
AnalyserNodetransformsmOutputBufferin place — repeated reads compound the distortion.In
GetFloatFrequencyDataandGetByteFrequencyData,AudioFingerprintManager::ApplyTransformation(mOutputBuffer.Elements(), …)mutates the cached buffer directly.mOutputBufferis reused across calls (with smoothing applied against the previous frame in standard AnalyserNode behavior), so:
- Two reads on the same frame multiply samples by the LCG-derived
multipliertwice, producing drift proportional to call count.- Smoothing feeds transformed values back into the next FFT result, so even single reads per frame accumulate distortion over time.
Apply the transformation on a copy destined for the JS array instead (the
ProcessDatacallback already has the outputSpan— transform there), somOutputBufferremains the canonical untouched FFT state.GetByteTimeDomainDataalready does this correctly viatmpBuffer; the float-frequency / byte-frequency paths should follow the same pattern.🐛 Sketch of fix for the float-frequency path
- // Apply audio fingerprint transformation to frequency data - { - ... - uint32_t seed = AudioFingerprintManager::GetSeed(userContextId); - if (seed != 0) { - AudioFingerprintManager::ApplyTransformation(mOutputBuffer.Elements(), mOutputBuffer.Length(), seed); - } - } aArray.ProcessData([&](const Span<float>& aData, JS::AutoCheckCannotGC&&) { size_t length = std::min(size_t(aData.Length()), mOutputBuffer.Length()); + // ... existing copy into aData ... + uint32_t userContextId = /* derive as before */; + uint32_t seed = AudioFingerprintManager::GetSeed(userContextId); + if (seed != 0) { + AudioFingerprintManager::ApplyTransformation(aData.Elements(), length, seed); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/audio-fingerprint-manager.patch` around lines 246 - 345, GetFloatFrequencyData and GetByteFrequencyData currently call AudioFingerprintManager::ApplyTransformation on mOutputBuffer (mutating the cached FFT state), which causes compounded distortion across reads; instead, remove the in-place transformation and apply AudioFingerprintManager::ApplyTransformation to the per-call output span or a local copy inside the aArray.ProcessData callback so mOutputBuffer remains untouched. Concretely: in GetFloatFrequencyData and GetByteFrequencyData delete the pre-ProcessData block that mutates mOutputBuffer, and inside the ProcessData lambda (where you already compute length and have the destination Span), compute the seed via AudioFingerprintManager::GetSeed(...) the same way and call AudioFingerprintManager::ApplyTransformation on the callback span (or on a local copy created from mOutputBuffer) before copying into aData; keep GetByteTimeDomainData behavior as-is for reference.
191-211:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCross-context disable: writing to
userContextId=0locks context 0 out of its own setter.
AudioFingerprintManager::SetSeedunconditionally callsDisableFunction(aUserContextId)(see lines 33–39 ofAudioFingerprintManager.cpp). In this method you then callSetSeed(0, seed)wheneveruserContextId != 0, which:
- Overwrites context 0's
audioFingerprintSeed_0with whatever value context N supplied (information leak across user contexts).- Sets
audio_disabled_0 = true, sosetAudioFingerprintSeedis now hidden on context 0 windows even though context 0 never called it.Additionally,
GetSeeddoesn't fall back fromaudioFingerprintSeed_<N>toaudioFingerprintSeed_0, so the cross-write doesn't even buy you a fallback — it only causes the disable side-effect.🐛 Proposed fix
AudioFingerprintManager::SetSeed(userContextId, seed); - if (userContextId != 0) { - AudioFingerprintManager::SetSeed(0, seed); - } - AudioFingerprintManager::DisableFunction(userContextId); + // SetSeed already disables for `userContextId`; no need to touch context 0.If a cross-context fallback is actually desired, add a write path that stores the value without calling
DisableFunction, and haveGetSeedlook upaudioFingerprintSeed_0as a fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/audio-fingerprint-manager.patch` around lines 191 - 211, nsGlobalWindowInner::SetAudioFingerprintSeed currently calls AudioFingerprintManager::SetSeed(0, seed) which triggers DisableFunction(0) and overwrites context 0 state (audioFingerprintSeed_0 and audio_disabled_0), causing context 0 to be locked out; instead, stop calling SetSeed(0, seed) from nsGlobalWindowInner when userContextId != 0 and either (a) add a new AudioFingerprintManager API (e.g. SetSeedFallback or SetSeedNoDisable) that writes audioFingerprintSeed_<id> without calling DisableFunction, or (b) modify AudioFingerprintManager::SetSeed to accept a flag to avoid DisableFunction for cross-context writes; also update AudioFingerprintManager::GetSeed to fallback to audioFingerprintSeed_0 when the requested context seed is absent so cross-context fallback works correctly.patches/screen-spoofing.patch (2)
78-107:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winShared
screen_disabled_%uflag silently disables the other setter.
DisabledKeyForUserContextreturns one key (screen_disabled_<ucid>), and the WebIDL Func guardIsFunctionEnabledForWebIDLis bound to bothsetScreenDimensionsandsetScreenColorDepthinWindow.webidl(lines 338–347). BecausensGlobalWindowInner::SetScreenDimensionsandSetScreenColorDepthboth callScreenDimensionManager::DisableFunction(userContextId), calling either method on a fresh context immediately hides both methods on subsequent window instances in that context.A page that needs to set width/height and color depth can never invoke the second call from a new window. Use a per-setter key.
🐛 Proposed fix
- static void DisableFunction(uint32_t userContextId); - static bool IsFunctionEnabledForWebIDL(JSContext* aCx, JSObject* aObj); + static void DisableDimensions(uint32_t userContextId); + static void DisableColorDepth(uint32_t userContextId); + static bool IsDimensionsEnabledForWebIDL(JSContext* aCx, JSObject* aObj); + static bool IsColorDepthEnabledForWebIDL(JSContext* aCx, JSObject* aObj);Then
Window.webidlshould reference the matchingFunc=for each method, and the setters should call their dedicated disable helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/screen-spoofing.patch` around lines 78 - 107, The shared key "screen_disabled_%u" causes calling one setter to disable the other; change the code to use a distinct per-setter key and helpers: replace DisabledKeyForUserContext with two specific key generators (e.g., DisabledKeyForDimensions(uint32_t) and DisabledKeyForColorDepth(uint32_t)) or add a suffix parameter, implement matching disable helpers (e.g., DisableDimensions/DisableColorDepth) instead of the single DisableFunction, and update IsFunctionEnabledForWebIDL to check the correct per-setter key; also update Window.webidl to reference the correct Func= guard for each of SetScreenDimensions and SetScreenColorDepth so each setter only disables its own flag.
307-324:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded
availTop = 25magic number.
GetAvailRectuses25as the default top inset and reduces available height by it. This silently bakes a platform assumption (macOS-style menu bar height) into every spoofed value. At minimum, name the constant; ideally, default to0and rely on the existingscreen.availTopMaskConfig, or derive it from the real platform chrome height.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/screen-spoofing.patch` around lines 307 - 324, In nsScreen::GetAvailRect, remove the hardcoded magic number 25 for availTop and instead use a named default or zero: call MaskConfig::GetInt32("screen.availTop") and if it has a value use that, otherwise set availTop to 0 (or to a well-named constant like DEFAULT_AVAIL_TOP = 0); update the code paths around nsGlobalWindowInner::Cast(inner), mozilla::dom::ScreenDimensionManager::GetUserContextIdFromWindow(win) and ScreenDimensionManager::GetDimensions(userContextId, &w, &h) to compute and return {0, availTop, w, h - availTop} using the new default variable rather than 25 so platform assumptions aren’t baked in.patches/webgl-spoofing.patch (1)
162-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCross-write to
userContextId = 0leaks vendor/renderer between contexts.
SetWebGLVendorandSetWebGLRendererboth do:WebGLParamsManager::SetVendor(userContextId, vendor); if (userContextId != 0) { WebGLParamsManager::SetVendor(0, vendor); }But
ClientWebGLContext::GetParameteronly readsWebGLParamsManager::GetVendor(GetUserContextId(), stored)— there's no fallback to context 0. So the write towebgl_vendor_0never affects reads in any other context; its only observable effect is that a default-context window now reads back whatever the most recent non-default context wrote (information leak across user contexts, and inconsistent with what context 0 itself would have observed before).Drop the
if (userContextId != 0) { … (0, vendor) }block in both setters, or — if a cross-context default is actually desired — makeGetVendor/GetRendererfall back to the_0key explicitly so the semantics are documented and symmetric.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/webgl-spoofing.patch` around lines 162 - 205, The setters nsGlobalWindowInner::SetWebGLVendor and ::SetWebGLRenderer should not write to the default context key (userContextId 0) because ClientWebGLContext::GetParameter only reads WebGLParamsManager::GetVendor/GetRenderer for the current GetUserContextId(), causing a cross-context leak; remove the conditional block that calls WebGLParamsManager::SetVendor(0, ...) and WebGLParamsManager::SetRenderer(0, ...), leaving the per-userContextId writes, leaving DisableVendorFunction/DisableRendererFunction and the JS self-destruct code unchanged; if you instead want symmetric fallback semantics, update WebGLParamsManager::GetVendor/GetRenderer to fall back to the 0 key explicitly and document the behavior, but do not perform the unconditional write from the setters.
🧹 Nitpick comments (7)
additions/juggler/TargetRegistry.js (1)
205-220: 💤 Low valueRedundant null check after polling loop with rejection.
The polling loop (lines 205-217) rejects the promise with an error if
gBrowseris not populated within 15 seconds. The subsequent check at line 218 (if (!domWindow.gBrowser)) is redundant because:
- If the polling loop times out, it rejects and the execution never reaches line 218
- If the polling loop succeeds,
gBrowseris guaranteed to be non-nullConsider removing lines 218-219 to eliminate dead code.
♻️ Simplify by removing redundant check
}); } - if (!domWindow.gBrowser) - return; const tabContainer = domWindow.gBrowser.tabContainer;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@additions/juggler/TargetRegistry.js` around lines 205 - 220, The redundant null check after the polling loop should be removed: the Promise used to await domWindow.gBrowser (created inside the if (!domWindow.gBrowser) block using start, tick, setTimeout and rejecting on timeout) either resolves when gBrowser is populated or rejects on timeout, so the subsequent if (!domWindow.gBrowser) return; is dead code; delete that check and directly use domWindow.gBrowser (e.g., proceed to const tabContainer = domWindow.gBrowser.tabContainer) to simplify the flow.additions/juggler/screencast/HeadlessWindowCapturer.h (1)
9-11: ⚡ Quick winRisk of pid_t redefinition conflict on Windows.
The
typedef int pid_t;workaround for Windows may cause redefinition errors ifpid_tis already defined by another included header (e.g.,<process.h>or other system headers). Consider guarding the typedef with a more robust check:`#ifdef` XP_WIN `#ifndef` _PID_T_ `#define` _PID_T_ typedef int pid_t; `#endif` `#endif`Or use a namespace-scoped type to avoid global namespace pollution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@additions/juggler/screencast/HeadlessWindowCapturer.h` around lines 9 - 11, The current Windows-only typedef int pid_t in HeadlessWindowCapturer.h can clash with other headers; update the fix by guarding the typedef so it only defines pid_t when not already defined (e.g., wrap the typedef in an `#ifndef` check) or replace the global typedef with a namespaced alias (avoid polluting global namespace) and update usages in the file (references to pid_t) to use the new safe symbol; ensure the change compiles on XP_WIN and preserves behavior for code expecting pid_t.patches/anti-font-fingerprinting.patch (1)
656-674: ⚡ Quick winAvoid exposing
mPresContextas a public field.Moving
mPresContextfromprivatetopublicinnsFontMetricsjust to letAutoTextRunaccess it breaks encapsulation and lets any caller mutate the raw pointer. Prefer a small public accessor and keep the member private.♻️ Proposed refactor
bool AllowForceGDIClassic() const { return mAllowForceGDIClassic; } `#endif` - // Pointer to the pres context for which this fontMetrics object was - // created. - nsPresContext* MOZ_NON_OWNING_REF mPresContext; + nsPresContext* PresContext() const { return mPresContext; } private: // Private destructor, to discourage deletion outside of Release(): ~nsFontMetrics(); @@ const nsFont mFont; RefPtr<gfxFontGroup> mFontGroup; RefPtr<nsAtom> const mLanguage; + // Pointer to the pres context for which this fontMetrics object was + // created. + nsPresContext* MOZ_NON_OWNING_REF mPresContext; const int32_t mP2A;And update
AutoTextRunto useaMetrics->PresContext()instead ofaMetrics->mPresContext.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/anti-font-fingerprinting.patch` around lines 656 - 674, nsFontMetrics exposes mPresContext as a public field which breaks encapsulation and allows callers (like AutoTextRun) to mutate the raw pointer; revert mPresContext to private, add a small public accessor on nsFontMetrics named PresContext() that returns the nsPresContext* (or MOZ_NON_OWNING_REF nsPresContext*), and update any callers (e.g. AutoTextRun) to call aMetrics->PresContext() instead of accessing aMetrics->mPresContext.patches/audio-fingerprint-manager.patch (1)
246-345: ⚡ Quick winDuplicated user-context-id resolution block.
The same ~10-line block resolving
userContextIdfromAudioContext → GlobalObject → Window/Workeris copy-pasted four times acrossAnalyserNodegetters. Extract a smallstaticor member helper (e.g.uint32_t ResolveUserContextId(AudioContext*)) to keep the call sites readable and prevent the four copies from drifting independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/audio-fingerprint-manager.patch` around lines 246 - 345, The duplicated block that resolves userContextId appears in AnalyserNode::GetFloatFrequencyData, GetByteFrequencyData, GetFloatTimeDomainData, and GetByteTimeDomainData; extract it into a single helper (e.g. static uint32_t ResolveUserContextId(AudioContext* aContext) or a private AnalyserNode::ResolveUserContextId() member) that encapsulates: getting the AudioContext via GetParentObject(), obtaining nsIGlobalObject and nsPIDOMWindowInner, falling back to GetCurrentThreadWorkerPrivate(), and returning the mUserContextId (or 0). Replace each pasted block with a call to that helper, then call AudioFingerprintManager::GetSeed(ResolveUserContextId(...)) and AudioFingerprintManager::ApplyTransformation as before.patches/webgl-spoofing.patch (3)
510-523: ⚡ Quick winVariable declaration inside a
caselabel without braces.The new
case LOCAL_GL_COMPRESSED_TEXTURE_FORMATS:declaresstd::vector<uint32_t> compressedTextureUint32directly under the label without an enclosing block. This works only because it's the last case before}, but it's brittle (a latercase/defaultadded below would make this ill-formed because the jump would skip a non-trivial initialization) and several compilers (-Wswitch-unreachable, MSVCC2360) flag it. Wrap the case body in braces.♻️ Proposed fix
- case LOCAL_GL_COMPRESSED_TEXTURE_FORMATS: + case LOCAL_GL_COMPRESSED_TEXTURE_FORMATS: { std::vector<uint32_t> compressedTextureUint32( state.mCompressedTextureFormats.begin(), state.mCompressedTextureFormats.end()); retval.set(Create<dom::Uint32Array>( cx, this, MaskConfig::MParamGLVector<uint32_t>(pname, compressedTextureUint32, mIsWebGL2), rv)); return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/webgl-spoofing.patch` around lines 510 - 523, The case for LOCAL_GL_COMPRESSED_TEXTURE_FORMATS declares a local std::vector (compressedTextureUint32) directly under the case label which can cause ill-formed code when other cases are added; fix it by enclosing the case body in braces so the vector and the subsequent call to retval.set(Create<dom::Uint32Array>(..., MaskConfig::MParamGLVector<uint32_t>(pname, compressedTextureUint32, mIsWebGL2), rv)) have their own scope, leaving the existing symbols (compressedTextureUint32, MaskConfig::MParamGLVector, retval.set, Create<dom::Uint32Array>, mIsWebGL2) unchanged.
255-275: 💤 Low valueWorker path falls through to
userContextId = 0when window lookup fails.In
GetUserContextId, themOffscreenCanvasbranch only consultsGetCurrentThreadWorkerPrivate()whendo_QueryInterface(global)succeeds in giving a non-nullnsPIDOMWindowInneris false and we're inside thatif (win)else-branch — butwinis checked insideif (global), so ifglobalis null we never reach the worker path. For OffscreenCanvas transferred to a worker,GetOwnerGlobal()should return the worker's global, anddo_QueryInterface(global)tonsPIDOMWindowInnerreturns null, so the current shape does fall through to the worker check — but only because of anelse ifchain that's easy to break. Add a unit test or at minimum a brief comment so a future reorder doesn't silently regress worker spoofing touserContextId = 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/webgl-spoofing.patch` around lines 255 - 275, GetUserContextId's mOffscreenCanvas branch currently only tries the worker path inside the else of the "if (win)" that itself is nested under "if (global)", so a null global can skip the WorkerPrivate check and fall through to returning 0; refactor the mOffscreenCanvas handling in GetUserContextId so you always attempt to resolve a WorkerPrivate via GetCurrentThreadWorkerPrivate() if do_QueryInterface(global) returns nullptr (i.e., try nsPIDOMWindowInner* win = do_QueryInterface(global); if win use win->GetBrowsingContext(), else independently call GetCurrentThreadWorkerPrivate() and return wp->GetOriginAttributes().mUserContextId), and add a short comment by the WorkerPrivate check (or a unit test) noting why the worker-path must be evaluated even when global is null to prevent future regressions.
369-423: ⚖️ Poor tradeoffOptimize parameter masking checks to avoid repeated JSON lookups on every
getParametercall.
GLParam()performs nested JSON lookups viaGetNested()on every invocation, even when no WebGL masking is configured. While config parsing is cached viastd::call_once, the.contains()checks and JSON access still run per-call. SincegetParameteris invoked extremely frequently by WebGL apps (per-frame, sometimes per-draw), consider early-exiting if no mask configuration keys are set, or caching a flag indicating whether any WebGL parameter masking is configured.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/webgl-spoofing.patch` around lines 369 - 423, The current GetParameter path calls MaskConfig::GLParam and does nested JSON lookups on every invocation; add a cached boolean (e.g. MaskConfig::HasWebGLParameterMasks or similar) computed once (using the existing call_once/config init) that indicates whether any WebGL parameter masks or related keys exist, and early-return/skip calling MaskConfig::GLParam and MaskConfig::CheckBool when that flag is false; update MaskConfig to compute and expose this flag during its initialization and change ClientWebGLContext::GetParameter to check the cached flag before invoking MaskConfig::GLParam or doing per-call contains/lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f05c3982-bec1-47ea-ad67-a038421377b7
📒 Files selected for processing (33)
README.mdadditions/juggler/TargetRegistry.jsadditions/juggler/TargetRegistry.js.bakadditions/juggler/screencast/HeadlessWindowCapturer.cppadditions/juggler/screencast/HeadlessWindowCapturer.hadditions/juggler/screencast/ScreencastEncoder.hadditions/juggler/screencast/nsScreencastService.cppbuild-tester/scripts/grading.pydocs/per-context-patches.mdpatches/anti-font-fingerprinting.patchpatches/audio-fingerprint-manager.patchpatches/config.patchpatches/font-hijacker.patchpatches/geolocation-spoofing.patchpatches/librewolf/mozilla_dirs.patchpatches/librewolf/ui-patches/hide-default-browser.patchpatches/locale-spoofing.patchpatches/macos-sdk-26.1-allow.patchpatches/playwright/0-playwright.patchpatches/screen-spoofing.patchpatches/system-ui-font-spoofing.patchpatches/timezone-spoofing.patchpatches/webgl-spoofing.patchpatches/windows-theming-bug-modified.patchpythonlib/camoufox/fingerprint-presets-v150.jsonpythonlib/camoufox/fingerprints.pypythonlib/camoufox/utils.pyscripts/_mixin.pyservice-tester/README.mdservice-tester/_proxies.pyservice-tester/run_tests.pyservice-tester/run_tests.shupstream.sh
| async cancelDownload(options) { | ||
| this._downloadInterceptor.cancelDownload(options.uuid); | ||
| } |
There was a problem hiding this comment.
Missing await causes cancelDownload to resolve before the underlying cancel completes.
DownloadInterceptor.cancelDownload (line 96) is async and awaits externalAppHandler.cancel(...). Here the outer TargetRegistry.cancelDownload does not await it, so:
- the returned promise resolves immediately, before the cancellation actually finishes, breaking callers that rely on the operation being complete (e.g. test/CDP teardown sequences); and
- any rejection from
externalAppHandler.cancelbecomes an unhandled promise rejection rather than propagating to the caller.
🛠 Suggested fix
async cancelDownload(options) {
- this._downloadInterceptor.cancelDownload(options.uuid);
+ await this._downloadInterceptor.cancelDownload(options.uuid);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@additions/juggler/TargetRegistry.js.bak` around lines 277 - 279,
TargetRegistry.cancelDownload currently calls
this._downloadInterceptor.cancelDownload(options.uuid) without awaiting it,
causing the outer promise to resolve prematurely and swallow rejections; update
TargetRegistry.cancelDownload to await (or return) the promise from
DownloadInterceptor.cancelDownload so callers get completion and any errors
propagate (refer to TargetRegistry.cancelDownload and
DownloadInterceptor.cancelDownload/externalAppHandler.cancel).
| async _newPageInternal({browserContextId}) { | ||
| console.error(`[TR-DEBUG] _newPageInternal start ctx=${browserContextId}`); | ||
| const browserContext = this.browserContextForId(browserContextId); | ||
| const features = "chrome,dialog=no,all"; | ||
| // See _callWithURIToLoad in browser.js for the structure of window.arguments | ||
| // window.arguments[1]: unused (bug 871161) | ||
| // [2]: referrerInfo (nsIReferrerInfo) | ||
| // [3]: postData (nsIInputStream) | ||
| // [4]: allowThirdPartyFixup (bool) | ||
| // [5]: userContextId (int) | ||
| // [6]: originPrincipal (nsIPrincipal) | ||
| // [7]: originStoragePrincipal (nsIPrincipal) | ||
| // [8]: triggeringPrincipal (nsIPrincipal) | ||
| // [9]: allowInheritPrincipal (bool) | ||
| // [10]: csp (nsIContentSecurityPolicy) | ||
| // [11]: nsOpenWindowInfo | ||
| const args = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray); | ||
| const urlSupports = Cc["@mozilla.org/supports-string;1"].createInstance( | ||
| Ci.nsISupportsString | ||
| ); | ||
| urlSupports.data = 'about:blank'; | ||
| args.appendElement(urlSupports); // 0 | ||
| args.appendElement(undefined); // 1 | ||
| args.appendElement(undefined); // 2 | ||
| args.appendElement(undefined); // 3 | ||
| args.appendElement(undefined); // 4 | ||
| const userContextIdSupports = Cc[ | ||
| "@mozilla.org/supports-PRUint32;1" | ||
| ].createInstance(Ci.nsISupportsPRUint32); | ||
| userContextIdSupports.data = browserContext.userContextId; | ||
| args.appendElement(userContextIdSupports); // 5 | ||
| args.appendElement(undefined); // 6 | ||
| args.appendElement(undefined); // 7 | ||
| args.appendElement(Services.scriptSecurityManager.getSystemPrincipal()); // 8 | ||
|
|
||
| console.error(`[TR-DEBUG] opening window with url=${AppConstants.BROWSER_CHROME_URL}`); | ||
| const window = Services.ww.openWindow(null, AppConstants.BROWSER_CHROME_URL, '_blank', features, args); | ||
| console.error(`[TR-DEBUG] openWindow returned, awaiting ready`); | ||
| await waitForWindowReady(window); | ||
| console.error(`[TR-DEBUG] window ready, browsers=${window.gBrowser ? window.gBrowser.browsers.length : 'no gBrowser'}`); | ||
| if (window.gBrowser.browsers.length !== 1) | ||
| throw new Error(`Unexpected number of tabs in the new window: ${window.gBrowser.browsers.length}`); | ||
| const browser = window.gBrowser.browsers[0]; | ||
| let target = this._browserToTarget.get(browser); | ||
| console.error(`[TR-DEBUG] initial target lookup: ${target ? 'found' : 'not found'}`); | ||
| let attempts = 0; | ||
| while (!target) { | ||
| attempts++; | ||
| console.error(`[TR-DEBUG] awaiting TargetCreated event (attempt ${attempts})`); | ||
| await helper.awaitEvent(this, TargetRegistry.Events.TargetCreated); | ||
| target = this._browserToTarget.get(browser); | ||
| console.error(`[TR-DEBUG] after TargetCreated: ${target ? 'matched' : 'not matched'}`); | ||
| } |
There was a problem hiding this comment.
Remove [TR-DEBUG] console.error statements before merge.
_newPageInternal is sprinkled with seven console.error('[TR-DEBUG] …') calls (lines 323, 357, 359, 361, 366, 370, 373). These look like ad-hoc instrumentation left over from chasing the Playwright issue 34586 timing problem. They will spam stderr on every page creation in production builds and leak internal state (context id, attempt counters, chrome URL). Drop them, or replace with a single gated dump() behind an env/pref check if you want a knob for future debugging.
🧹 Proposed cleanup
async _newPageInternal({browserContextId}) {
- console.error(`[TR-DEBUG] _newPageInternal start ctx=${browserContextId}`);
const browserContext = this.browserContextForId(browserContextId);
@@
- console.error(`[TR-DEBUG] opening window with url=${AppConstants.BROWSER_CHROME_URL}`);
const window = Services.ww.openWindow(null, AppConstants.BROWSER_CHROME_URL, '_blank', features, args);
- console.error(`[TR-DEBUG] openWindow returned, awaiting ready`);
await waitForWindowReady(window);
- console.error(`[TR-DEBUG] window ready, browsers=${window.gBrowser ? window.gBrowser.browsers.length : 'no gBrowser'}`);
if (window.gBrowser.browsers.length !== 1)
throw new Error(`Unexpected number of tabs in the new window: ${window.gBrowser.browsers.length}`);
const browser = window.gBrowser.browsers[0];
let target = this._browserToTarget.get(browser);
- console.error(`[TR-DEBUG] initial target lookup: ${target ? 'found' : 'not found'}`);
- let attempts = 0;
while (!target) {
- attempts++;
- console.error(`[TR-DEBUG] awaiting TargetCreated event (attempt ${attempts})`);
await helper.awaitEvent(this, TargetRegistry.Events.TargetCreated);
target = this._browserToTarget.get(browser);
- console.error(`[TR-DEBUG] after TargetCreated: ${target ? 'matched' : 'not matched'}`);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@additions/juggler/TargetRegistry.js.bak` around lines 322 - 374, The
_newPageInternal function contains multiple console.error('[TR-DEBUG] ...')
debug prints that must be removed before merge; locate the debug calls inside
_newPageInternal (references: browserContextId, AppConstants.BROWSER_CHROME_URL,
waitForWindowReady, window, this._browserToTarget, and the loop awaiting
TargetRegistry.Events.TargetCreated) and delete those console.error lines, or
replace them with a single conditional debug output (e.g., use dump() or a
logger) gated by a runtime env/pref check so it does not spam stderr in
production.
| @@ -2507,6 +2509,22 @@ nsresult nsGlobalWindowOuter::SetNewDocument(Document* aDocument, | ||
| MOZ_ALWAYS_SUCCEEDS(bc->SetCurrentInnerWindowId(mInnerWindow->WindowID())); | ||
| } | ||
|
|
||
| + // Camoufox: Auto-apply stored timezone when navigating to new document | ||
| + // This ensures timezone persists across page navigations within same context | ||
| + { | ||
| + uint32_t userContextId = 0; | ||
| + if (bc) { | ||
| + userContextId = bc->OriginAttributesRef().mUserContextId; | ||
| + } | ||
| + nsAutoString storedTimezone; | ||
| + if (mozilla::dom::TimezoneManager::GetTimezone(userContextId, storedTimezone)) { | ||
| + if (JSContext* cx = nsContentUtils::GetCurrentJSContext()) { | ||
| + NS_LossyConvertUTF16toASCII timezoneASCII(storedTimezone); | ||
| + JS::SetRealmTimeZoneOverride(cx, timezoneASCII.get()); | ||
| + } | ||
| + } | ||
| + } |
There was a problem hiding this comment.
SetNewDocument ignores JS::SetRealmTimeZoneOverride failures.
The auto-apply-on-navigate path discards the return value of JS::SetRealmTimeZoneOverride. If the stored timezone is corrupt or no longer a valid TZ id (e.g. after an ICU update), the new realm silently runs with no override, while TimezoneManager still reports it as set. At least log a warning, or Remove() the stored value so the next call recomputes from MaskConfig.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patches/timezone-spoofing.patch` around lines 218 - 236, In SetNewDocument
after obtaining storedTimezone via mozilla::dom::TimezoneManager::GetTimezone
and before/after calling JS::SetRealmTimeZoneOverride, check its boolean return
and handle failures: if JS::SetRealmTimeZoneOverride(cx, timezoneASCII.get())
returns false, log a warning (include storedTimezone and userContextId) and call
the TimezoneManager removal API (e.g.
mozilla::dom::TimezoneManager::Remove(userContextId) or the equivalent method)
so the invalid stored value is cleared; keep using
nsContentUtils::GetCurrentJSContext and the storedTimezone/ timezoneASCII
variables to locate the code to change. Ensure logging uses the existing logging
facility (NS_WARNING/ProcessLogger/MOZ_LOG as appropriate) rather than silently
discarding the failure.
| return "aarch64-unknown-linux-gnu" if arch == "arm64" else f"{arch}-pc-linux-gnu" | ||
| if target == "windows": | ||
| return f"{arch}-pc-mingw32" | ||
| return f"{arch}-pc-windows-msvc" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Firefox 150 require MSVC instead of MinGW for Windows builds?
💡 Result:
No. Firefox 150 does not require MSVC instead of MinGW for Windows builds. What the Firefox build docs and build-system metadata indicate is that MSVC is no longer the Tier-1 Windows build compiler; instead, Windows/x86 and Windows/x86-64 are Tier-1 targets that use clang-cl, while MinGW is a supported Tier-2 option (e.g., mingw-clang on Windows/x86, and mingw-gcc was listed as unsupported due to clang-bindgen requirements). The Mozilla build setup for Windows also centers on the MozillaBuild environment plus the ability to pass the desired target/ toolchain via mozconfig (for example, you can pass a MinGW target like x86_64-pc-windows-gnu/mingw-w64). Evidence: - Mozilla’s supported build targets list Tier-1 Windows/x86 and Windows/x86-64 as using clang-cl, and explicitly states “Prior to Firefox 63, Windows/x86 and Windows/x86-64 relied on the MSVC compiler; from Firefox 63 onward MSVC is not supported.” It then lists Tier-2 Windows/x86 (mingw-clang) and Windows/x86-64 (mingw-gcc, “Unsupported due to requirements for clang-bindgen”).[1] - A Firefox build job/mozconfig example for MinGW clang shows setting “--target=x86_64-pc-windows-gnu” and using an x86_64-w64-mingw32-clang toolchain, with MinGW-specific notes in the file.[2] - Mozilla’s Windows build instructions describe using the MozillaBuild shell and running./mach build, rather than mandating MSVC specifically.[3][4] So, for Windows builds of Firefox 150, MSVC is not a requirement; MinGW-based builds are possible (with caveats per the supported-targets matrix), while the primary supported Windows compiler is clang-cl.
Citations:
- 1: http://gecko-docs.mozilla.org-l1.s3.us-west-2.amazonaws.com/contributing/build/supported.html
- 2: https://searchfox.org/firefox-main/source/browser/config/mozconfigs/win64/mingwclang
- 3: https://firefox-source-docs.mozilla.org/setup/windows_build.html
- 4: https://firefox-source-docs.mozilla.org/_sources/setup/windows_build.rst.txt
Remove or reject the change from MinGW to MSVC build target.
Firefox 150 does not support MSVC for Windows builds. Mozilla's official documentation explicitly states that MSVC support was removed starting with Firefox 63. The current supported Windows compilers are clang-cl (Tier-1) and mingw-clang (Tier-2). Changing from MinGW to MSVC will break Windows builds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/_mixin.py` at line 61, Revert the Windows target change in
scripts/_mixin.py: the return currently using the MSVC target must be changed
back to the supported MinGW/clang target so Windows builds remain compatible;
locate the return statement that constructs the target triple (the line
returning the "{arch}-pc-windows-msvc" string) and restore it to the previous
Windows target triple (e.g., the MinGW/clang variant such as
"{arch}-pc-windows-gnu"/mingw target) or otherwise reject the msVC change so
builds keep using clang-cl or mingw-clang as documented by Mozilla.
1 participant
Related Issue
Closes #
Description
Type of Change
Testing
Fingerprint Report
Please submit a report from both the service tester and build tester.
Fingerprint report
Checklist
Service tests pass (for python library changes) -temporarily out of service lol./service-tester/run_tests.sh --browser-version official/prerelease/146.0.1-alpha.25(attach screenshot)./build-tester/run_tests.shA score of at least 1000 must be achieved (attach screenshot)Summary by CodeRabbit
Release Notes
New Features
fingerprint_preset=TrueconfigurationsetAudioFingerprintSeed()APIBug Fixes
Documentation
Chores