Process crash on startupcache-invalidate notification from WebExtension Experiments
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | unaffected |
thunderbird_esr91 | --- | unaffected |
firefox-esr78 | --- | unaffected |
firefox-esr91 | --- | unaffected |
firefox93 | --- | unaffected |
firefox94 | --- | fixed |
firefox95 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
(Regression)
Details
(Keywords: regression)
Attachments
(2 files)
1.07 KB,
application/x-xpinstall
|
Details | |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
(In reply to John Bieling (:TbSync) from bug 1718481 comment #8)
Created attachment 9246118 [details]
test.xpiThe changes introduced in this bug cause Firefox to crash, if WebExtension Experiments flush the cash using:
Services.obs.notifyObservers(null, "startupcache-invalidate", null);
This method has been working before and is the only supported and "preferred" way to do it. See instructions given in https://bugzilla.mozilla.org/show_bug.cgi?id=703720#c0
Crash observed on Windows 10.
Steps to reproduce:
- Set
xpinstall.signatures.required
tofalse
- Set
extensions.experiments.enabled
totrue
- Restart Firefox (to make sure the changed config is picked up)
- Install the attached add-on
- Wait 10 seconds
- Disable the add-on in Add-ons Manager
- Wait 10 seconds (until the add-on is moved to the "Disabled" section)
- Open Hamburger Menu -> Settings/Preferences
- Observe Firefox crash.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
Assignee | ||
Comment 2•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
The issue is that nsMessageManagerScriptExecutor::sCachedScripts
holds stencils that's retrieved from ScriptPreloader::GetChildSingleton().GetCachedStencil(...)
, that can be stencil backed by on-memory XDR buffer (not mmaped XDR buffer),
and the on-memory XDR buffer is cleared on "startupcache-invalidate" notification.
The solutions are either:
- (a) Do not cache stencils from
ScriptPreloader
innsMessageManagerScriptExecutor::sCachedScripts
, but only cache when the stencil is compiled innsMessageManagerScriptExecutor::TryCacheLoadAndCompileScript
- (b) Purge
nsMessageManagerScriptExecutor::sCachedScripts
on "startupcache-invalidate" notification.
I'm leaning toward (a), given caching the same stencil in 2 places doesn't make much sense.
Assignee | ||
Comment 4•3 years ago
|
||
Use nsMessageManagerScriptExecutor::sCachedScripts only for cacing stencils
compiled inside nsMessageManagerScriptExecutor::TryCacheLoadAndCompileScript.
Updated•3 years ago
|
Comment 7•3 years ago
|
||
bugherder |
Assignee | ||
Comment 8•3 years ago
|
||
Comment on attachment 9246266 [details]
Bug 1736039 - Do not cache stencils returned from ScriptPreloader into nsMessageManagerScriptExecutor::sCachedScripts. r?kmag!
Beta/Release Uplift Approval Request
- User impact if declined: Process crash when using WebExtension Experiments
Verified the fix in the m-c binary from treeherder - Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This removes duplication of cache.
The cache still works and the content is same. - String changes made/needed:
Comment 9•3 years ago
|
||
Comment on attachment 9246266 [details]
Bug 1736039 - Do not cache stencils returned from ScriptPreloader into nsMessageManagerScriptExecutor::sCachedScripts. r?kmag!
Approved for 94.0rc1. Thanks for adding a test.
Comment 10•3 years ago
|
||
bugherder uplift |
Description
•