1370087 - Crash in mozilla::dom::StorageUtils::GenerateOriginKey

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[[:philipp]]

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

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

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!

Comment 2

[Marcia Knous [:marcia]]

Currently this is the #4 top browser crash on Nightly - 45 installs/140 crashes in the last 7 days.

Comment 3

[[:philipp]]

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 4

[Sylvestre Ledru [:Sylvestre]]

Hector, can you help with that?

Comment 5

[Hector Zhao [:hectorz]]

I think it's from this call [1] in [e-mail]

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

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Hector, are you going to work on a fix? If not, do you know who could help? 
Thanks

Comment 7

[Hector Zhao [:hectorz]]

(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.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

OK, to be clear, because we no longer have aurora, we need a fix asap (and 55 move to beta next week).
Merci!

Comment 9

[Hector Zhao [:hectorz]]

(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

[Hector Zhao [:hectorz]]

(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!

Comment 11

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

As Hector is taking this bug, it's fine to remove my NI on :baku. Thanks Hector.

Comment 12

[Nicholas Nethercote [inactive]]

This is #3 Windows topcrash in Nightly 20170609030207.

Comment 13

[Hector Zhao [:hectorz]]

(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.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: storage.patch

I also agree that we should add a check in GenerateOriginKey.

Comment 15

[Hector Zhao [:hectorz]]

(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.

Comment 16

[Gerry Chang [:gchang]]

Mark this as 55 blocking as topcrash and we want to take this in 55.0b1.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8876616 [details] [diff] [review]
storage.patch

I guess so. Though other option is for the caller to do null check.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3d6ffed3b5
GenerateOriginKey cannot assert the existence of nsIPrincipal, r=smaug

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 20

[Andrea Marchesini [:baku]]

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

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/72a0956cd5c1

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3d3d6ffed3b5

Comment 24

[Andrei Vaida [:avaida]]

(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?

Comment 25

[Andrea Marchesini [:baku]]

Comment 3: Addon "China Edition Addons Manager" Version = 2.54 should trigger the crash.

Comment 26

[Hector Zhao [:hectorz]]

(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

[Andrei Vaida [:avaida]]

Thank you Andrea, Hector. Flagging this for manual verification.

Comment 28

[Ciprian Georgiu, Desktop QA]

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.