Bug 144474 - DOM bindings should not be using a reference type to point to a temporary object
Summary: DOM bindings should not be using a reference type to point to a temporary object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-30 15:21 PDT by Oliver Hunt
Modified: 2015-05-01 12:15 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.94 KB, patch)
2015-04-30 15:27 PDT, Oliver Hunt
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2015-04-30 15:21:54 PDT
DOM bindings should not be using a reference type to point to a temporary object
Comment 1 Oliver Hunt 2015-04-30 15:27:16 PDT
Created attachment 252096 [details]
Patch
Comment 2 Oliver Hunt 2015-04-30 15:41:44 PDT
Committed r183648: <http://trac.webkit.org/changeset/183648>
Comment 3 Alexey Proskuryakov 2015-04-30 17:06:50 PDT
This broke bindings tests (need new results landed).

I'm not sure if analysis in this patch is accurate, references sometimes do extend the lifetime of an object, even though that's counter-intuitive.
Comment 4 Darin Adler 2015-05-01 07:23:31 PDT
I don’t think this patch is correct; I would like more information about the problem here, and how the problem was solved. Alexey’s point is one possible source of misunderstanding that I am suspecting.
Comment 5 Darin Adler 2015-05-01 07:24:28 PDT
I’m particularly concerned about the change to %nativeType. I believe the change to the local variable pointing to existing_name is unnecessary but harmless.
Comment 6 Oliver Hunt 2015-05-01 07:56:52 PDT
native type is used to define the type used for a local, the bug here occurs when we assign a temporary to a local reference.

It does result in incorrect behavior and this trivially provable by making refptr clear the pointer reference in its destructor. The outcome is a huge number of tests failing courtesy of references to dead refptrs
Comment 7 Alexey Proskuryakov 2015-05-01 12:15:33 PDT
Here is the change that this patch made in generated code: <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/4924/steps/bindings-generation-tests/logs/stdio>.

The reference used to be initialized from a ternary operator, which complicates things. Oliver told me that he is working on a minimal C++ test demonstrating that a reference doesn't extend object lifetime in this case (either correctly, or due to a clang bug).