WebCore/dom/Element.cpp setBooleanAttribute(); ExceptionCode ex; removeAttribute(name, ex); removeAttribute(): void Element::removeAttribute(const QualifiedName& name, ExceptionCode& ec) { if (namedAttrMap) { namedAttrMap->removeNamedItem(name, ec); if (ec == NOT_FOUND_ERR) ec = 0; } } removeNamedItem() doesn't set any value to ec if the specified attribute exists. So, if removeAttribute() is called by setBooleanAttribute(), uninitialized ec can be referred at "if (ec == NOT_FOUND_ERR)". Note: This never makes a real bug because ex in setBooleanAttribute() is not used after removeAttribute() call. Valgrind complaints about this.
Created attachment 41398 [details] Proposed patch
Wouldn’t it make more sense to initialize the variable at the level that it matters? setBooleanAttribute doesn’t care about the exception code so there’s no need for it to do anything with it. After your change the code in removeAttribute will still do the wrong thing if called by another caller.
(In reply to comment #2) > Wouldn’t it make more sense to initialize the variable at the level that it > matters? setBooleanAttribute doesn’t care about the exception code so there’s > no need for it to do anything with it. After your change the code in > removeAttribute will still do the wrong thing if called by another caller. I think our convention for ExceptionCode is "initialize it with 0 when it is declared."
> I think our convention for ExceptionCode is "initialize it with 0 when it is > declared." So, if removeAttribute() set 0 before removenamedItem() and other callers initialize ec with 0, we would have redundant initialization.
(In reply to comment #3) > (In reply to comment #2) > > Wouldn’t it make more sense to initialize the variable at the level that it > > matters? setBooleanAttribute doesn’t care about the exception code so there’s > > no need for it to do anything with it. After your change the code in > > removeAttribute will still do the wrong thing if called by another caller. > > I think our convention for ExceptionCode is "initialize it with 0 when it is > declared.” A quick grep in WebCore for “ExceptionCode ec;” shows that is not the case.
Comment on attachment 41398 [details] Proposed patch As Mark said - if a caller doesn't plan to look at ExceptionCode, it can pass an uninitialized value. See bug 21939 for more detailed discussion of the current policy (especially comments 3 and 12 there).
Created attachment 41400 [details] Proposed patch (rev.2) ok, I follow it. Updated the patch so that removeAttribute() initializes ec.
Comment on attachment 41400 [details] Proposed patch (rev.2) r=me It may make sense to add a comment explaining that this assignment is only needed to please tools like valgrind - since it's useless otherwise, people will be tempted to remove it.
Comment on attachment 41400 [details] Proposed patch (rev.2) Let commit bot land it
Comment on attachment 41400 [details] Proposed patch (rev.2) Rejecting patch 41400 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11468 test cases. http/tests/navigation/postredirect-reload.html -> crashed Exiting early after 1 failures. 8689 tests run. 390.95s total testing time 8688 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output
Comment on attachment 41400 [details] Proposed patch (rev.2) That crash looks unrelated to your patch. I've filed bug 30519. Adding this back to the commit-queue.
Comment on attachment 41400 [details] Proposed patch (rev.2) Clearing flags on attachment: 41400 Committed r49792: <http://trac.webkit.org/changeset/49792>
All reviewed patches have been landed. Closing bug.