1490339 - HomePage.jsm tries to open about:home (or user-set homepage) as a .properties file

Status: NEW

(Firefox :: General, defect, P3)

Description

[:Gijs (he/him)]

I instrumented .properties and .dtd loads for some fluent conversion work.

I noticed that we attempt to load about:home as a properties bundle.

C++ stack:

  * frame #0: 0x0000000101c2881d XUL`nsStringBundleBase::ParseProperties(this=0x000000011e864d80, aProps=0x000000011e864df8) at nsStringBundle.cpp:470 [opt]
    frame #1: 0x0000000101c28d39 XUL`nsStringBundle::GetStringImpl(this=0x000000011e864d80, aName=0x00007ffeefbfd330, aResult=0x00007ffeefbfd478) at nsStringBundle.cpp:611 [opt]
    frame #2: 0x0000000101c28cca XUL`nsStringBundleBase::GetStringFromName(this=0x000000011e864d80, aName="browser.startup.homepage", aResult=0x00007ffeefbfd478) at nsStringBundle.cpp:605 [opt]
    frame #3: 0x0000000101c03098 XUL`nsPrefBranch::GetDefaultFromPropertiesFile(this=<unavailable>, aPrefName="browser.startup.homepage", aReturn=0x00007ffeefbfd478) at Preferences.cpp:3113 [opt]
    frame #4: 0x0000000101c02def XUL`nsPrefBranch::GetComplexValue(this=0x0000000112a8f0a0, aPrefName="browser.startup.homepage", aType=0x0000000119190180, aRetVal=0x00007ffeefbfd6d8) at Preferences.cpp:2545 [opt]
    frame #5: 0x0000000101bee33e XUL`NS_InvokeByIndex at xptcinvoke_asm_x86_64_unix.S:129
    frame #6: 0x00000001023c5149 XUL`XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [inlined] CallMethodHelper::Invoke(this=<unavailable>) at XPCWrappedNative.cpp:1675 [opt]
    frame #7: 0x00000001023c5129 XUL`XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [inlined] CallMethodHelper::Call(this=<unavailable>) at XPCWrappedNative.cpp:1233 [opt]
    frame #8: 0x00000001023c45a2 XUL`XPCWrappedNative::CallMethod(ccx=<unavailable>, mode=<unavailable>) at XPCWrappedNative.cpp:1200 [opt]
    frame #9: 0x00000001023c6603 XUL`XPC_WN_CallMethod(cx=<unavailable>, argc=2, vp=<unavailable>) at XPCWrappedNativeJSOps.cpp:911 [opt]
    frame #10: 0x000000010534c7bb XUL`js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [inlined] CallJSNative(native=<unavailable>)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at Interpreter.cpp:448 [opt]
    frame #11: 0x000000010534c67f XUL`js::InternalCallOrConstruct(cx=0x0000000113825000, args=0x00007ffeefbfdbb8, construct=<unavailable>) at Interpreter.cpp:536 [opt]
    frame #12: 0x0000000105346f97 XUL`Interpret(JSContext*, js::RunState&) [inlined] js::CallFromStack(cx=<unavailable>) at Interpreter.cpp:593 [opt]
    frame #13: 0x0000000105346f8f XUL`Interpret(cx=<unavailable>, state=0x00007ffeefbfdfc0) at Interpreter.cpp:3265 [opt]
    frame #14: 0x000000010533e59b XUL`js::RunScript(cx=0x0000000113825000, state=0x00007ffeefbfdfc0) at Interpreter.cpp:428 [opt]
    frame #15: 0x000000010534caf7 XUL`js::InternalCallOrConstruct(cx=0x0000000113825000, args=0x00007ffeefbfe038, construct=<unavailable>) at Interpreter.cpp:560 [opt]
    frame #16: 0x000000010534d7d7 XUL`js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] InternalCall(cx=<unavailable>) at Interpreter.cpp:587 [opt]
    frame #17: 0x000000010534d779 XUL`js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) at Interpreter.cpp:606 [opt]
    frame #18: 0x000000010534d76a XUL`js::CallGetter(cx=0x0000000113825000, thisv=JS::HandleValue @ r15, getter=JS::HandleValue @ r12, rval=JS::MutableHandleValue @ r14) at Interpreter.cpp:726 [opt]
    frame #19: 0x0000000105801641 XUL`js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at NativeObject.cpp:2122 [opt]
    frame #20: 0x00000001058015aa XUL`js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) [inlined] bool GetExistingProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) at NativeObject.cpp:2175 [opt]
    frame #21: 0x00000001058014e3 XUL`js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at NativeObject.cpp:2391 [opt]
    frame #22: 0x0000000105800bd5 XUL`js::NativeGetProperty(cx=0x0000000113825000, obj=<unavailable>, receiver=JS::HandleValue @ 0x00007ffeefbfe0e0, id=<unavailable>, vp=JS::MutableHandleValue @ 0x00007ffeefbfe148) at NativeObject.cpp:2427 [opt]
    frame #23: 0x000000010534fd43 XUL`js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) [inlined] js::GetProperty(cx=<unavailable>, receiver=<unavailable>, id=<unavailable>, vp=<unavailable>) at NativeObject.h:1705 [opt]
    frame #24: 0x000000010534fd16 XUL`js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) [inlined] js::GetProperty(obj=<unavailable>, receiver=<unavailable>) at JSObject.h:784 [opt]
    frame #25: 0x000000010534fcf6 XUL`js::GetProperty(cx=0x0000000113825000, v=<unavailable>, name=<unavailable>, vp=<unavailable>) at Interpreter.cpp:4643 [opt]
    frame #26: 0x0000000105344476 XUL`Interpret(JSContext*, js::RunState&) [inlined] GetPropertyOperation(cx=<unavailable>, fp=<unavailable>, pc=<unavailable>) at Interpreter.cpp:219 [opt]
    frame #27: 0x0000000105344120 XUL`Interpret(cx=<unavailable>, state=0x00007ffeefbfe810) at Interpreter.cpp:2982 [opt]
    frame #28: 0x000000010533e59b XUL`js::RunScript(cx=0x0000000113825000, state=0x00007ffeefbfe810) at Interpreter.cpp:428 [opt]
    frame #29: 0x000000010534caf7 XUL`js::InternalCallOrConstruct(cx=0x0000000113825000, args=0x00007ffeefbfe8a0, construct=<unavailable>) at Interpreter.cpp:560 [opt]
    frame #30: 0x000000010534cfc9 XUL`js::Call(cx=<unavailable>, fval=<unavailable>, thisv=<unavailable>, args=0x00007ffeefbfe8a0, rval=<unavailable>) at Interpreter.cpp:606 [opt]
    frame #31: 0x00000001056b0778 XUL`JS_CallFunctionValue(cx=0x0000000113825000, obj=<unavailable>, fval=<unavailable>, args=0x00007ffeefbfeb40, rval=JS::MutableHandleValue @ 0x00007ffeefbfe928) at jsapi.cpp:2807 [opt]
    frame #32: 0x00000001023bdc66 XUL`nsXPCWrappedJSClass::CallMethod(this=0x000000011e862fc0, wrapper=<unavailable>, methodIndex=<unavailable>, info=<unavailable>, nativeParams=0x00007ffeefbfedb0) at XPCWrappedJSClass.cpp:1166 [opt]
    frame #33: 0x0000000101bef70b XUL`::PrepareAndDispatch(self=0x000000011e8658e0, methodIndex=<unavailable>, args=<unavailable>, gpregs=0x00007ffeefbfee70, fpregs=0x00007ffeefbfeea0) at xptcstubs_x86_64_darwin.cpp:129 [opt]
    frame #34: 0x0000000101bee52b XUL`SharedStub + 91
    frame #35: 0x000000010501e8d2 XUL`nsCommandLine::EnumerateHandlers(this=0x00000001007dd140, aCallback=(XUL`EnumRun(nsICommandLineHandler*, nsICommandLine*, void*) at nsCommandLine.cpp:575), aClosure=0x0000000000000000)(nsICommandLineHandler*, nsICommandLine*, void*), void*) at nsCommandLine.cpp:525 [opt]
    frame #36: 0x000000010501ef4b XUL`nsCommandLine::Run(this=0x00000001007dd140) at nsCommandLine.cpp:588 [opt]
    frame #37: 0x000000010520c9ae XUL`XREMain::XRE_mainRun(this=0x00007ffeefbff210) at nsAppRunner.cpp:4723 [opt]
    frame #38: 0x000000010520d01c XUL`XREMain::XRE_main(this=0x00007ffeefbff210, argc=<unavailable>, argv=<unavailable>, aConfig=0x00007ffeefbff3b0) at nsAppRunner.cpp:4927 [opt]
    frame #39: 0x000000010520d6b1 XUL`XRE_main(argc=<unavailable>, argv=<unavailable>, aConfig=<unavailable>) at nsAppRunner.cpp:5019 [opt]
    frame #40: 0x000000010000110b firefox`main [inlined] do_main(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>) at nsBrowserApp.cpp:233 [opt]
    frame #41: 0x0000000100001029 firefox`main(argc=<unavailable>, argv=<unavailable>, envp=0x00007ffeefbff830) at nsBrowserApp.cpp:315 [opt]
    frame #42: 0x00007fff54c75015 libdyld.dylib`start + 1


This is the JS stack:

0 getHomepagePref() ["resource:///modules/HomePage.jsm":27]
1 get() ["resource:///modules/HomePage.jsm":47]
    this = [object Object]
2 get defaultArgs() ["file:///Users/gkruitbosch/dev/builds/opt/dist/Nightly.app/Contents/Resources/browser/components/nsBrowserContentHandler.js":571]
    this = [object Object]
3 openBrowserWindow(cmdLine = [xpconnect wrapped nsICommandLine], triggeringPrincipal = [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable)]) ["file:///Users/gkruitbosch/dev/builds/opt/dist/Nightly.app/Contents/Resources/browser/components/nsBrowserContentHandler.js":190]
4 dch_handle(cmdLine = [xpconnect wrapped nsICommandLine]) ["file:///Users/gkruitbosch/dev/builds/opt/dist/Nightly.app/Contents/Resources/browser/components/nsBrowserContentHandler.js":785]
    this = [object Object]



There are a few issues here:
1) we don't cache the pref, so we do this every time someone asks for the default homepage (e.g. every time a new window opens)
2) localized prefs work by reading the actual pref which points to a .properties file (hopefully) which then gets loaded. We should avoid doing this here.
3) the getlocalizedpref backend seems to throw away stringbundle instances if they fail to parse, so we create a new instance, parse the URI and then fail (because it's not in the allowed schemes list) every time someone runs this code.
4) if the user changes the pref, we're going to run this code for https://www.mozilla.org/ (or whatever the user sets as a homepage). No bueno.

Hopefully we can fix this by getting the pref value first and just using it unless we're confident it's a .properties file.

Comment 1

[:Gijs (he/him)]

Zibi, to fix this bug as filed, can/should we just change the order of the two pref lookups at https://searchfox.org/mozilla-central/rev/aa9a7136835deb0eeba00c62bb50a4a0e2cdea2d/browser/modules/HomePage.jsm#38-49 and only do the localized pref one if the result of the string/char pref fetch ends in .properties ?

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Seems like it should work!

May it also be related to bug 1695248 ?