If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash in mozilla::dom::StorageUtils::GenerateOriginKey

VERIFIED FIXED in Firefox 55

Status

()

Core
DOM
P1
critical
VERIFIED FIXED
4 months ago
3 months ago

People

(Reporter: philipp, Assigned: baku)

Tracking

({crash, regression})

55 Branch
mozilla56
All
Windows
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55blocking verified, firefox56 verified)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
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
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)
Currently this is the #4 top browser crash on Nightly - 45 installs/140 crashes in the last 7 days.
(Reporter)

Comment 3

4 months 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
Hector, can you help with that?
Flags: needinfo?(bzhao)
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)
Hector, are you going to work on a fix? If not, do you know who could help? 
Thanks
Flags: needinfo?(bzhao)
(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)
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
(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?
(Reporter)

Updated

4 months ago
Blocks: 1369323
(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)
As Hector is taking this bug, it's fine to remove my NI on :baku. Thanks Hector.
Flags: needinfo?(amarchesini)
This is #3 Windows topcrash in Nightly 20170609030207.

Updated

3 months ago
Priority: -- → P1
(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

3 months ago
Created attachment 8876616 [details] [diff] [review]
storage.patch

I also agree that we should add a check in GenerateOriginKey.
Attachment #8876616 - Flags: review?(bugs)
(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)
Mark this as 55 blocking as topcrash and we want to take this in 55.0b1.
tracking-firefox55: --- → blocking

Comment 17

3 months 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

3 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3d6ffed3b5
GenerateOriginKey cannot assert the existence of nsIPrincipal, r=smaug

Comment 19

3 months ago
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

3 months 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 21

3 months ago
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

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/72a0956cd5c1
status-firefox55: affected → fixed

Comment 23

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d3d6ffed3b5
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(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

3 months ago
Comment 3: Addon "China Edition Addons Manager" Version = 2.54 should trigger the crash.
Flags: needinfo?(amarchesini)
(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
Thank you Andrea, Hector. Flagging this for manual verification.
Flags: qe-verify? → qe-verify+
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
status-firefox55: fixed → verified
status-firefox56: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.