1092333 - nsDOMStringMap::NamedDeleter doesn't always write its out-parameter

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

which leads to the following Valgrind complaint for the test case
dom/imptests/html/html/dom/elements/global-attributes/test_dataset-delete.html

Conditional jump or move depends on uninitialised value(s)
   at 0x64D426B: mozilla::dom::DOMStringMapBinding::DOMProxyHandler::delete_(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) const (ff-O2-linux64/dom/bindings/DOMStringMapBinding.cpp:438)
   by 0x81362C4: js::Proxy::delete_(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) (js/src/proxy/Proxy.cpp:186)
   by 0x813636C: js::proxy_DeleteGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) (js/src/proxy/Proxy.cpp:724)
   by 0x81788C8: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2319)
   by 0x817DBE4: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:432)
   by 0x817E31E: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:501)
   by 0x80CEFA1: js_fun_apply(JSContext*, unsigned int, JS::Value*) (js/src/jsfun.cpp:1310)
   by 0x817E3DE: CallJSNative (js/src/jscntxtinlines.h:231)
   by 0x817E3DE: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:482)
   by 0x81757CE: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2537)
   by 0x817DBE4: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:432)
   by 0x817DDD2: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) (js/src/vm/Interpreter.cpp:638)
   by 0x817DF33: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (js/src/vm/Interpreter.cpp:675)
   by 0x8090126: Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::Value*) (js/src/jsapi.cpp:4737)
   by 0x6310F9D: nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) (dom/base/nsJSUtils.cpp:246)
   by 0x631116A: nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) (dom/base/nsJSUtils.cpp:312)
   by 0x6E2FF7A: nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, JS::SourceBufferHolder&, void**) (content/base/src/nsScriptLoader.cpp:1128)

Comment 1

[Julian Seward [:jseward]]

Attachment: Ensure all return paths set |found|

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8515237 [details] [diff] [review]
Ensure all return paths set |found|

r=me

Comment 3

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/954632f3b2d6

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/954632f3b2d6

Comment 5

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

Comment 6

[Boris Zbarsky [:bzbarsky]]

Julian, can you please request branch approvals for this?

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Please request approvals ASAP. I'd like a summary of why we need to take this in 34, which is at the very end of the Beta cycle. We are going to spin beta10 on Monday, which is an extra beta outside of the normal schedule. If we're going to take this, I'd like it uplifted before then.

Comment 8

[Julian Seward [:jseward]]

Comment on attachment 8515237 [details] [diff] [review]
Ensure all return paths set |found|

Approval Request Comment

I just found the problem.  I have no understanding of the code nor of
the significance of the problem.  So am leaving the fields mostly blank.

[Feature/regressing bug #]:

[User impact if declined]:

[Describe test coverage new/current, TBPL]: -b do -p all -u all -t none looked ok:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2abcaf954cb0

[Risks and why]:

[String/UUID change made/needed]:
none

Comment 9

[Boris Zbarsky [:bzbarsky]]

That main impact is that deleting from a DOMStringMap will have a random return value, which seems uncool.  The problem has been around since bug 803129.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

This looks like a pretty simple fix but given that this issue has been present since Firefox 19, unless there is some new pressure to fix this bug I think that we can ship the fix in Firefox 35 instead of taking a fix at the end of the Firefox 34 cycle.

bz - Do you think that we need to take this fix in Firefox 34?

Comment 11

[Boris Zbarsky [:bzbarsky]]

We can probably live with skipping 34.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8515237 [details] [diff] [review]
Ensure all return paths set |found|

OK. Let's take this fix in 35. 

Beta-
Aurora+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/120b2b6378ea