WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
160830
Binding NULL pointer to reference in WebCore::RenderObject
https://bugs.webkit.org/show_bug.cgi?id=160830
Summary
Binding NULL pointer to reference in WebCore::RenderObject
Jonathan Bedard
Reported
2016-08-12 15:52:12 PDT
Location: WebCore/css/PropertySetCSSStyleDeclaration.cpp:92 It appears possible that m_nextObject can be NULL. In this event, the function commitLineBreakAtCurrentWidth(...) is an illegal call because it requires a reference. Note that even when m_nextObject is NULL, commitLineBreakAtCurrentWidth(...) still needs to be called.
Attachments
Patch
(6.90 KB, patch)
2016-08-12 15:57 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(11.54 KB, patch)
2016-08-15 10:41 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(11.57 KB, patch)
2016-08-16 13:31 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(11.79 KB, patch)
2016-08-18 12:07 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2
(689.26 KB, application/zip)
2016-08-18 14:26 PDT
,
Build Bot
no flags
Details
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Jonathan Bedard
Comment 1
2016-08-12 15:57:57 PDT
Created
attachment 285969
[details]
Patch
Jonathan Bedard
Comment 2
2016-08-12 16:01:12 PDT
Note that this change aimed for minimum impact on existing code. Investigation of commitLineBreakAtCurrentWidth(...) indicates that the code path called by this function handles and expects NULL pointers for RenderObjects, and one notable caller of commitLineBreakAtCurrentWidth(...) passes NULL RenderObjects in certain circmstances.
Daniel Bates
Comment 3
2016-08-12 16:36:21 PDT
Comment on
attachment 285969
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=285969&action=review
> Source/WebCore/rendering/line/BreakingContext.h:170 > + void commitLineBreakAtCurrentWidth(RenderObject* object, unsigned offset = 0, int nextBreak = -1)
I understand the motivation for changing from a pointer to a reference to fix the compiler warning on line 1228 in BreakingContext::commitAndUpdateLineBreakIfNeeded(), but is this preferred way to allow moving to the end of the iterator (assuming the code in BreakingContext::commitAndUpdateLineBreakIfNeeded() is correct)? Does anyone ever call this function or InlineIterator::moveTo(RenderObject*, unsigned, int) with a null RenderObject and a non-zero offset? What does it mean to support such calls?
Jonathan Bedard
Comment 4
2016-08-15 10:41:20 PDT
Created
attachment 286066
[details]
Patch
Jonathan Bedard
Comment 5
2016-08-15 10:48:59 PDT
A few note about the underlying architecture: The central issue resolved in this patch is that the iterator can point to nothing (by design) functions which access the WebCore::InlineIterator generally assume the iterator is always pointing to something. Additionally, when assigning this iterator to nothing, calls were made which assume the iterator was being assigned to a reference. This patch attempts to unify assumptions, allowing the iterator to explicitly be assigned to NULL.
Myles C. Maxfield
Comment 6
2016-08-15 10:55:38 PDT
Comment on
attachment 286066
[details]
Patch Please add a rest which causes the bull reference to be created.
Myles C. Maxfield
Comment 7
2016-08-15 10:56:27 PDT
(In reply to
comment #6
)
> Comment on
attachment 286066
[details]
> Patch > > Please add a rest which causes the bull reference to be created.
*null reference (Sorry for the typo)
Myles C. Maxfield
Comment 8
2016-08-15 11:00:23 PDT
(In reply to
comment #7
)
> (In reply to
comment #6
) > > Comment on
attachment 286066
[details]
> > Patch > > > > Please add a rest which causes the bull reference to be created. > > *null reference > > (Sorry for the typo)
*test Wow, I am not typing well today....
Jonathan Bedard
Comment 9
2016-08-15 14:11:13 PDT
(In reply to
comment #8
)
> Please add a test which causes the null reference to be created.
The tests actually already exist, js/dom/script-start-end-locations.html and js/dom/cross-frame-symbols.html, for example, both cause this to happen (these are by no means an exhaustive list). This bug was caught with Clang's undefined behavior sanitizer, which I am running locally. Currently, for a number of reasons, we cannot run the undefined behavior sanitizer on our build machines. Since this bug was the result of converting a NULL pointer to a reference and then back to a pointer, the above tests did not cause a crash. I'm not sure how to test this, unless the undefined behavior sanitizer is integrated to our testing framework, which is a possible long term plan, but won't happen in the near future.
Myles C. Maxfield
Comment 10
2016-08-16 11:20:37 PDT
Comment on
attachment 286066
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=286066&action=review
> Source/WebCore/rendering/line/BreakingContext.h:546 > + if (m_current.renderer())
if (auto* renderer = m_current.renderer()) commitLineBreakAtCurrentWidth(*renderer);
Jonathan Bedard
Comment 11
2016-08-16 13:31:35 PDT
Created
attachment 286199
[details]
Patch
WebKit Commit Bot
Comment 12
2016-08-18 10:42:15 PDT
Comment on
attachment 286199
[details]
Patch Rejecting
attachment 286199
[details]
from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 286199, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: unk #8 succeeded at 944 (offset -1 lines). Hunk #9 succeeded at 1021 (offset -1 lines). Hunk #10 succeeded at 1038 (offset -1 lines). Hunk #11 succeeded at 1098 (offset -1 lines). Hunk #12 succeeded at 1236 (offset -1 lines). Hunk #13 succeeded at 1274 (offset -1 lines). 2 out of 13 hunks FAILED -- saving rejects to file Source/WebCore/rendering/line/BreakingContext.h.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output:
http://webkit-queues.webkit.org/results/1893939
Jonathan Bedard
Comment 13
2016-08-18 12:07:53 PDT
Created
attachment 286383
[details]
Patch
Jonathan Bedard
Comment 14
2016-08-18 12:09:47 PDT
Comment on
attachment 286383
[details]
Patch Offset had been changed to Optional<unsigned> from int, the most recent patch fixes conflicts.
Build Bot
Comment 15
2016-08-18 14:26:00 PDT
Comment on
attachment 286383
[details]
Patch
Attachment 286383
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/1894581
New failing tests: fast/scrolling/ios/scrollTo-at-page-load.html
Build Bot
Comment 16
2016-08-18 14:26:04 PDT
Created
attachment 286395
[details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Jonathan Bedard
Comment 17
2016-08-18 16:57:08 PDT
There was a bug in EWS, the first bot timed out while running tests.
WebKit Commit Bot
Comment 18
2016-08-18 18:03:28 PDT
Comment on
attachment 286383
[details]
Patch Clearing flags on attachment: 286383 Committed
r204620
: <
http://trac.webkit.org/changeset/204620
>
WebKit Commit Bot
Comment 19
2016-08-18 18:03:33 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug