Bug 195962 - Remove copyRef() calls added in r243163
Summary: Remove copyRef() calls added in r243163
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks: 195920
  Show dependency treegraph
 
Reported: 2019-03-19 13:56 PDT by Michael Catanzaro
Modified: 2019-03-20 10:09 PDT (History)
14 users (show)

See Also:


Attachments
Patch (4.12 KB, patch)
2019-03-19 15:34 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.92 MB, application/zip)
2019-03-19 19:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (3.04 MB, application/zip)
2019-03-19 19:58 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-03-19 13:56:56 PDT
I added some calls to copyRef() in r243163 that Chris wants removed, see bug #195920.
Comment 1 Michael Catanzaro 2019-03-19 15:34:06 PDT
Created attachment 365245 [details]
Patch
Comment 2 Build Bot 2019-03-19 15:36:56 PDT
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 3 Build Bot 2019-03-19 19:00:31 PDT
Comment on attachment 365245 [details]
Patch

Attachment 365245 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11573478

New failing tests:
js/dom/custom-constructors.html
Comment 4 Build Bot 2019-03-19 19:00:42 PDT
Created attachment 365286 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 5 Build Bot 2019-03-19 19:58:35 PDT
Comment on attachment 365245 [details]
Patch

Attachment 365245 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11574224

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 6 Build Bot 2019-03-19 19:58:37 PDT
Created attachment 365297 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 7 Chris Dumez 2019-03-20 09:13:43 PDT
Comment on attachment 365245 [details]
Patch

Clearing flags on attachment: 365245

Committed r243204: <https://trac.webkit.org/changeset/243204>
Comment 8 Chris Dumez 2019-03-20 09:13:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-03-20 09:14:46 PDT
<rdar://problem/49063670>
Comment 10 Darin Adler 2019-03-20 09:55:05 PDT
Comment on attachment 365245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365245&action=review

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator_templates.py:235
> +            Ref<JSON::Object> jsonResult = m_result.releaseNonNull();
> +            auto result = WTFMove(*reinterpret_cast<Ref<${objectType}>*>(&jsonResult));
> +            return result;

I would think we’d need to add a comment explaining that this is to work around a GCC 9 bug. Otherwise, someone could refactor this to get rid of the local variable. Another way to protect against that is to make sure we have at least EWS testing completion with GCC 9.
Comment 11 Michael Catanzaro 2019-03-20 10:09:13 PDT
(In reply to Darin Adler from comment #10)
> I would think we’d need to add a comment explaining that this is to work
> around a GCC 9 bug. Otherwise, someone could refactor this to get rid of the
> local variable. Another way to protect against that is to make sure we have
> at least EWS testing completion with GCC 9.

I almost added a comment, but in the end I decided it was low-value since if it's ever refactored, we'll either get a warning again and know right away, or else we won't (if GCC stops warning there) and it will be fine.

EWS won't catch it unless we enable -Werror. There are pros and cons to enabling -Werror on the EWS. The risk is that it will probably significantly increase the number of build failures. The benefit is that it will surely reduce the number of compiler warnings introduced.

(I agree it would be nice to have EWS testing more supported versions of GCC.)