Closed
Bug 1370087
Opened 5 years ago
Closed 5 years ago
Crash in mozilla::dom::StorageUtils::GenerateOriginKey
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | blocking | verified |
firefox56 | --- | verified |
People
(Reporter: philipp, Assigned: baku)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
902 bytes,
patch
|
smaug
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-69b9124d-2244-4587-8edb-ac28e0170604. ============================================================= Crashing Thread (0) Frame Module Signature Source 0 xul.dll mozilla::dom::StorageUtils::GenerateOriginKey(nsIPrincipal*, nsACString&, nsACString&) dom/storage/StorageUtils.cpp:19 1 xul.dll mozilla::dom::LocalStorageManager::GetStorageInternal(mozilla::dom::LocalStorageManager::CreateMode, mozIDOMWindow*, nsIPrincipal*, nsAString const&, bool, nsIDOMStorage**) dom/storage/LocalStorageManager.cpp:220 2 xul.dll mozilla::dom::LocalStorageManager::PrecacheStorage(nsIPrincipal*, nsIDOMStorage**) dom/storage/LocalStorageManager.cpp:269 3 xul.dll NS_InvokeByIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54 4 xul.dll XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:982 5 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:470 6 xul.dll InternalCall js/src/vm/Interpreter.cpp:515 7 xul.dll Interpret js/src/vm/Interpreter.cpp:3028 8 xul.dll js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:410 9 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:488 10 xul.dll js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:534 11 xul.dll CallGetter js/src/vm/NativeObject.cpp:1912 12 xul.dll GetPropertyOperation js/src/vm/Interpreter.cpp:193 13 xul.dll Interpret js/src/vm/Interpreter.cpp:2743 14 xul.dll js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:410 15 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:488 16 xul.dll InternalCall js/src/vm/Interpreter.cpp:515 17 xul.dll js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/proxy/Wrapper.cpp:166 18 xul.dll js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/proxy/CrossCompartmentWrapper.cpp:353 19 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:452 20 xul.dll InternalCall js/src/vm/Interpreter.cpp:515 21 xul.dll Interpret js/src/vm/Interpreter.cpp:3028 22 xul.dll js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:410 23 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:488 24 xul.dll InternalCall js/src/vm/Interpreter.cpp:515 25 xul.dll JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:2832 26 xul.dll nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJSClass.cpp:1214 27 xul.dll nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJS.cpp:615 28 xul.dll PrepareAndDispatch xpcom/reflect/xptcall/md/win32/xptcstubs.cpp:85 29 xul.dll SharedStub xpcom/reflect/xptcall/md/win32/xptcstubs.cpp:112 30 xul.dll NS_InvokeByIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54 crash reports with this signature started showing up in 55.0a1 build 20170603030204. this would be the changelog to the day before: https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2017-06-02&tochange=43039280fe464869428f03b047bb7c762784f44b
Comment 1•5 years ago
|
||
Hey :baku, I noticed your footprints around the StorageUtils and LocalStorageManager code in May (though the dates don't exactly match the potential regression range 2016 June 3rd), I am still wondering perhaps you know where this regressed from? Thanks!
Flags: needinfo?(amarchesini)
Comment 2•5 years ago
|
||
Currently this is the #4 top browser crash on Nightly - 45 installs/140 crashes in the last 7 days.
Reporter | ||
Comment 3•5 years ago
|
||
the crash seems to be correlated to chinese builds: (96.80% in signature vs 00.55% overall) Addon "China Edition Addons Manager" Version = 2.54
Comment 5•5 years ago
|
||
I think it's from this call [1] in cehomepage@mozillaonline.com Maybe a result of the removal of nsScriptSecurityManager::GetNoAppCodebasePrincipal in bug 1369323? [1]: https://dxr.mozilla.org/addons/source/addons/636266/chrome_ntab/modules/NTabDB.jsm#76
Flags: needinfo?(bzhao)
Comment 6•5 years ago
|
||
Hector, are you going to work on a fix? If not, do you know who could help? Thanks
Flags: needinfo?(bzhao)
Comment 7•5 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #6) > Hector, are you going to work on a fix? If not, do you know who could help? > Thanks I'll be working on the fix.
Flags: needinfo?(bzhao)
Comment 8•5 years ago
|
||
OK, to be clear, because we no longer have aurora, we need a fix asap (and 55 move to beta next week). Merci!
Assignee: nobody → bzhao
Comment 9•5 years ago
|
||
(In reply to Hector Zhao [:hectorz] from comment #7) > (In reply to Sylvestre Ledru [:sylvestre] from comment #6) > > Hector, are you going to work on a fix? If not, do you know who could help? > > Thanks > > I'll be working on the fix. I'm thinking of a fix in our extension when saying this. Bug 1369323 happens to make my code pass undefined as nsIPrincipal to nsIDOMStorageManager.precacheStorage. I'm not sure, maybe this should also be fixed in m-c, with a check of valid nsIPrincipal in it?
Comment 10•5 years ago
|
||
(In reply to Hector Zhao [:hectorz] from comment #9) > > I'm thinking of a fix in our extension when saying this. We'll release an update to this extension later today to fix the symptom. > > Bug 1369323 happens to make my code pass undefined as nsIPrincipal to > nsIDOMStorageManager.precacheStorage. I'm not sure, maybe this should also > be fixed in m-c, with a check of valid nsIPrincipal in it? :sylvestre, please redirect this bug if you agree such a fix is necessary, thanks!
Flags: needinfo?(sledru)
Comment 11•5 years ago
|
||
As Hector is taking this bug, it's fine to remove my NI on :baku. Thanks Hector.
Flags: needinfo?(amarchesini)
![]() |
||
Comment 12•5 years ago
|
||
This is #3 Windows topcrash in Nightly 20170609030207.
Updated•5 years ago
|
Priority: -- → P1
Comment 13•5 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #12) > This is #3 Windows topcrash in Nightly 20170609030207. We just pushed the extension update live. (In reply to Hsin-Yi Tsai [:hsinyi] from comment #11) > As Hector is taking this bug, it's fine to remove my NI on :baku. Thanks > Hector. As I said, I don't think my fix is the proper fix here. Other code could potentially make the same mistake and call nsIDOMStorageManager.precacheStorage with undefined as nsIPrincipal and trigger the same crash.
Assignee | ||
Comment 14•5 years ago
|
||
I also agree that we should add a check in GenerateOriginKey.
Attachment #8876616 -
Flags: review?(bugs)
Comment 15•5 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #14) > > I also agree that we should add a check in GenerateOriginKey. Thanks! Bug re-assigned to you.
Assignee: bzhao → amarchesini
Flags: needinfo?(sledru)
Comment 16•5 years ago
|
||
Mark this as 55 blocking as topcrash and we want to take this in 55.0b1.
tracking-firefox55:
--- → blocking
Comment 17•5 years ago
|
||
Comment on attachment 8876616 [details] [diff] [review] storage.patch I guess so. Though other option is for the caller to do null check.
Attachment #8876616 -
Flags: review?(bugs) → review+
Comment 18•5 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3d6ffed3b5 GenerateOriginKey cannot assert the existence of nsIPrincipal, r=smaug
Hi Baku, Andrew, could you please nominate this patch for uplift to Beta asap? This is a top crasher on 55. Assuming this is a low risk and (hopefully) right fix, I will wait for this fix to land before I gtb 55.0b1.
Flags: needinfo?(overholt)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 20•5 years ago
|
||
Comment on attachment 8876616 [details] [diff] [review] storage.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1369323 [User impact if declined]: Extensions can make gecko to crash [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: would be nice. yes. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: low. I just added to avoid the use of a null principal. [Why is the change risky/not risky?]: just a simple check. [String changes made/needed]: none
Flags: needinfo?(overholt)
Flags: needinfo?(amarchesini)
Attachment #8876616 -
Flags: approval-mozilla-beta?
Comment on attachment 8876616 [details] [diff] [review] storage.patch Given that this is a top crasher on Nightly55 and a low-risk fix, let's take it in 55.0b1.
Attachment #8876616 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/72a0956cd5c1
Comment 23•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d3d6ffed3b5
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 24•5 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #20) > [Is this code covered by automated tests?]: no > [Has the fix been verified in Nightly?]: not yet > [Needs manual test from QE? If yes, steps to reproduce]: would be nice. yes. Hi Andrea, could you please expand on what manual QA can do here to help? Do we know of a specific extension that we could use to reproduce the crash on an affected build?
Flags: qe-verify?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 25•5 years ago
|
||
Comment 3: Addon "China Edition Addons Manager" Version = 2.54 should trigger the crash.
Flags: needinfo?(amarchesini)
Comment 26•5 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #25) > Comment 3: Addon "China Edition Addons Manager" Version = 2.54 should > trigger the crash. That's not accurate. It should be > (100.0% in signature vs 00.47% overall) Addon "China Edition Firefox Home Page" = true from [1]. Or, you can just run these > let storageManager = Cc["@mozilla.org/dom/localStorage-manager;1"].getService(Ci.nsIDOMStorageManager); > storageManager.precacheStorage(undefined); In the browser console. [1]: https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Adom%3A%3AStorageUtils%3A%3AGenerateOriginKey#correlations
Comment 27•5 years ago
|
||
Thank you Andrea, Hector. Flagging this for manual verification.
Flags: qe-verify? → qe-verify+
Updated•5 years ago
|
status-firefox54:
--- → unaffected
status-firefox-esr52:
--- → unaffected
I've reproduced this crash using an affected Nightly build 55.0a1 (2017-06-04) by running the following code: let storageManager = Cc["@mozilla.org/dom/localStorage-manager;1"].getService(Ci.nsIDOMStorageManager); storageManager.precacheStorage(undefined) in the brwoser console. - https://crash-stats.mozilla.com/report/index/4fb07263-46ec-472a-8e73-63f130170619 This crash is not repro anymore on 56.0a1 (2017-06-18) and 55.0b2 (20170615063713) under Windows 10 x64 and Windows 7 x86.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•