Bug 114672 – should null check mainResourceLoader() before use it

• Component: Page Loading
• Status: RESOLVED DUPLICATE

[Mary Wu]

in DocumentLoader, function mainResourceLoader() may return 0 in some cases, but there some places use it without null-check beforehand.

[Mary Wu]

Created attachment 198281 [details]
patch

[Mary Wu]

Comment on attachment 198281 [details]
patch

sorry, wrong attachment

[Mary Wu]

Created attachment 198314 [details]
patch

[Nate Chapin]

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.

[Mary Wu]

(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.

[Mary Wu]

cc

[Mary Wu]

Created attachment 198494 [details]
patch

or add assert there if MainResourceLoader() shouldn't be null, that would be better than do nothing, I think

[Nate Chapin]

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.

[Sam Weinig]

Comment on attachment 198494 [details]
patch

This should have a test case.

[Mary Wu]

(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.

[Mary Wu]

Created attachment 198717 [details]
add a test case, thanks

[Benjamin Poulain]

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.

[Mary Wu]

(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

[Mary Wu]

Created attachment 198842 [details]
with a drt test, thanks

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Mary Wu]

Created attachment 198925 [details]
fix style issue

[Mary Wu]

Created attachment 198928 [details]
format changeLog

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Mary Wu]

Created attachment 199214 [details]
fix result in mac

[Build Bot]

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

[Build Bot]

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

[Mary Wu]

Created attachment 199376 [details]
fix result in mac-wk2

[Mary Wu]

Created attachment 199622 [details]
remove redundant words in test case.

[Brent Fulgham]

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.

[Mary Wu]

(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: [e-mail] <[e-mail]@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:(

[Mary Wu]

dup

*** This bug has been marked as a duplicate of bug 116451 ***