in DocumentLoader, function mainResourceLoader() may return 0 in some cases, but there some places use it without null-check beforehand.
Created attachment 198281 [details] patch
Comment on attachment 198281 [details] patch sorry, wrong attachment
Created attachment 198314 [details] patch
Comment on attachment 198314 [details] patch Do you have a specific case where you're seeing a null dereference? I believe mainResourceLoader() is supposed to be guaranteed non-null at these points, but it's possible I missed a case.
(In reply to comment #4) > (From update of attachment 198314 [details]) > Do you have a specific case where you're seeing a null dereference? I believe mainResourceLoader() is supposed to be guaranteed non-null at these points, but it's possible I missed a case. yes, I met one case crash here: @@ -678,7 +678,7 @@ void DocumentLoader::continueAfterContentPolicy(PolicyAction policy) mainReceivedError(frameLoader()->client()->cannotShowURLError(m_request)); return; } - InspectorInstrumentation::continueWithPolicyDownload(m_frame, this, mainResourceLoader()->identifier(), m_response); + InspectorInstrumentation::continueWithPolicyDownload(m_frame, this, mainResourceLoader() ? mainResourceLoader()->identifier() : 0, m_response); If DocumentLoader was used to download, mainResourceLoader() would be null and crash 100% time. Though it's rare case that DocumentLoader was used to download, but you can't forbid user to use that way. Besides, since mainResourceLoader() was defined to possible return 0, I would prefer to have null-check to error-proof in any cases.
cc
Created attachment 198494 [details] patch or add assert there if MainResourceLoader() shouldn't be null, that would be better than do nothing, I think
Comment on attachment 198494 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=198494&action=review I don't suppose you have a test case to reproduce the crashes you're seeing? > Source/WebCore/loader/DocumentLoader.cpp:520 > + m_identifierForLoadWithoutResourceLoader = mainResourceLoader() ? mainResourceLoader()->identifier() : 0; I think this should be an assert instead. > Source/WebCore/loader/DocumentLoader.cpp:552 > + return; I think this should be an assert instead.
Comment on attachment 198494 [details] patch This should have a test case.
(In reply to comment #8) > (From update of attachment 198494 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198494&action=review > > I don't suppose you have a test case to reproduce the crashes you're seeing? I'll add one. > > > Source/WebCore/loader/DocumentLoader.cpp:520 > > + m_identifierForLoadWithoutResourceLoader = mainResourceLoader() ? mainResourceLoader()->identifier() : 0; > > I think this should be an assert instead. ok > > > Source/WebCore/loader/DocumentLoader.cpp:552 > > + return; > > I think this should be an assert instead. I hesitate to change to assert since randomly hit it one time.
Created attachment 198717 [details] add a test case, thanks
Comment on attachment 198717 [details] add a test case, thanks View in context: https://bugs.webkit.org/attachment.cgi?id=198717&action=review > Source/WebCore/ChangeLog:8 > + Add a manual test which will cause mainResourceLoader return null. ManualTests are bad. The changelog should explain why it cannot be tested in any other way.
(In reply to comment #12) > (From update of attachment 198717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198717&action=review > > > Source/WebCore/ChangeLog:8 > > + Add a manual test which will cause mainResourceLoader return null. > > ManualTests are bad. > The changelog should explain why it cannot be tested in any other way. will change it to drt test, thanks
Created attachment 198842 [details] with a drt test, thanks
Attachment 198842 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/http/tests/download/download-same-page-expected.txt', u'LayoutTests/http/tests/download/download-same-page.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/DocumentLoader.cpp']" exit_code: 1 Source/WebCore/ChangeLog:10: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Source/WebCore/ChangeLog:11: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 198842 [details] with a drt test, thanks Attachment 198842 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/111055 New failing tests: http/tests/download/download-same-page.html
Created attachment 198852 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Comment on attachment 198842 [details] with a drt test, thanks Attachment 198842 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/72654 New failing tests: fast/selectors/unqualified-hover-strict.html http/tests/download/download-same-page.html
Created attachment 198910 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Created attachment 198925 [details] fix style issue
Created attachment 198928 [details] format changeLog
Comment on attachment 198928 [details] format changeLog Attachment 198928 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/136261 New failing tests: http/tests/download/download-same-page.html
Created attachment 198930 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 198928 [details] format changeLog Attachment 198928 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/186321 New failing tests: http/tests/download/download-same-page.html
Created attachment 198934 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Created attachment 199214 [details] fix result in mac
Comment on attachment 199214 [details] fix result in mac Attachment 199214 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/207005 New failing tests: http/tests/download/download-same-page.html
Created attachment 199247 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Created attachment 199376 [details] fix result in mac-wk2
Created attachment 199622 [details] remove redundant words in test case.
Comment on attachment 199622 [details] remove redundant words in test case. View in context: https://bugs.webkit.org/attachment.cgi?id=199622&action=review Looks like a nice change. I had a few syntax corrections as well as some concerns about the clarify of the change. > LayoutTests/ChangeLog:3 > + should null check mainResourceLoader() before use it Should null check mainResourceLoader() before using it. > LayoutTests/ChangeLog:8 > + Add a test to make sure mainResourceLoader() null-check before used. .... mainResourceLoader() is null-checked before use. > Source/WebCore/ChangeLog:3 > + should null check mainResourceLoader() before use it Should null check mainResourceLoader() before using it. > Source/WebCore/loader/DocumentLoader.cpp:516 > + ASSERT(mainResourceLoader()); Why do we only assert here, where we provide actual protection in the other routines you modified? This would still be a crash, wouldn't it? > Source/WebCore/loader/DocumentLoader.cpp:681 > + InspectorInstrumentation::continueWithPolicyDownload(m_frame, this, mainResourceLoader() ? mainResourceLoader()->identifier() : 0, m_response); I think using this ternary operator inline in the method call is sort of hard to read. I would prefer to see something like: unsigned long identifier = mainResourceLoader() ? mainResourceLoader()->identifier() : 0; InspectorInstrumentation::continueWithPolicyDownload(m_frame, this, identifier, m_response); > Source/WebCore/loader/DocumentLoader.cpp:694 > + InspectorInstrumentation::continueWithPolicyIgnore(m_frame, this, mainResourceLoader() ? mainResourceLoader()->identifier() : 0, m_response); Ditto.
(In reply to comment #31) > (From update of attachment 199622 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199622&action=review > > Looks like a nice change. I had a few syntax corrections as well as some concerns about the clarify of the change. > > > LayoutTests/ChangeLog:3 > > + should null check mainResourceLoader() before use it > > Should null check mainResourceLoader() before using it. > > > LayoutTests/ChangeLog:8 > > + Add a test to make sure mainResourceLoader() null-check before used. > > .... mainResourceLoader() is null-checked before use. > > > Source/WebCore/ChangeLog:3 > > + should null check mainResourceLoader() before use it > > Should null check mainResourceLoader() before using it. > > > Source/WebCore/loader/DocumentLoader.cpp:516 > > + ASSERT(mainResourceLoader()); > > Why do we only assert here, where we provide actual protection in the other routines you modified? This would still be a crash, wouldn't it? > > > Source/WebCore/loader/DocumentLoader.cpp:681 > > + InspectorInstrumentation::continueWithPolicyDownload(m_frame, this, mainResourceLoader() ? mainResourceLoader()->identifier() : 0, m_response); > > I think using this ternary operator inline in the method call is sort of hard to read. I would prefer to see something like: > > unsigned long identifier = mainResourceLoader() ? mainResourceLoader()->identifier() : 0; > InspectorInstrumentation::continueWithPolicyDownload(m_frame, this, identifier, m_response); > > > Source/WebCore/loader/DocumentLoader.cpp:694 > > + InspectorInstrumentation::continueWithPolicyIgnore(m_frame, this, mainResourceLoader() ? mainResourceLoader()->identifier() : 0, m_response); > > Ditto. Thanks a lot for the review and comments, Brent. I found apple checked in similar patch recently: commit ef7981e25287a3dd2cb57087b126a44234423471 Author: andersca@apple.com <andersca@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc> Date: Thu May 23 20:08:21 2013 +0000 Crash in convertMainResourceLoadToDownload when downloading file by option-return https://bugs.webkit.org/show_bug.cgi?id=116451 since my test code was also based on the change, looks the patch became obsolete:(
dup *** This bug has been marked as a duplicate of bug 116451 ***