Bug 12463 – WebArchiver - attempt to insert nil exception when archive empty iframe

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jim Correia]

If you attempt to archive the main frame of a page like the attached one with an empty iframe, during the archiving process an exception will be raised:

     *** -[NSCFArray addObject:]: attempt to insert nil

WebResource's designated initializer returns nil if there is no mainResource, but the calling code doesn't handle this. Patch attached.

To reproduce archive the main frame of any product page loaded from amazon.com:

<http://www.amazon.com/Beginning-Tiger-Dashboard-Widget-Development/dp/0471778257/sr=8-2/qid=1170100387/ref=sr_1_2/105-9135809-2773235?ie=UTF8&s=books>

Reduced test case attached.

[Jim Correia]

Created attachment 12750 [details]
Test Case described in bug body.

[Jim Correia]

Created attachment 12751 [details]
Patch/fix.

[David Kilzer (:ddkilzer)]

The crash occurs when opening the archive, not when initially saving it.

Shipping Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8N1037) does NOT crash when opening such a webarchive.

Confirmed crash with locally-built debug build of WebKit r19232 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8N1037).

[Jim Correia]

Created attachment 12753 [details]
changelog

[David Kilzer (:ddkilzer)]

Created attachment 12754 [details]
Test (will crash ToT!)

[David Kilzer (:ddkilzer)]

Created attachment 12756 [details]
Stack trace

Note that this is appears to be crashing from a missing nil/null check in the new loader code that used to be "free" with the old ObjC loader code.

Another way to fix this bug is to add a nil check to the loader in the appropriate place.

[Jim Correia]

Dave - I think they are really two distinct issues. I have not tested the loading case at all.

The bug I am referring to is the result of an -addObject:nil being sent to a mutable array as the archive is created. This is corrected in my patch.

Do you need a sample application which demonstrates the problem, or is the description and patch sufficient?

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Dave - I think they are really two distinct issues. I have not tested the
> loading case at all.
> 
> The bug I am referring to is the result of an -addObject:nil being sent to a
> mutable array as the archive is created. This is corrected in my patch.
> 
> Do you need a sample application which demonstrates the problem, or is the
> description and patch sufficient?

Interesting!  What version of Safari/WebKit did you use to cause the crash when saving the webarchive?

[Jim Correia]

(In reply to comment #8)
> Interesting!  What version of Safari/WebKit did you use to cause the crash when
> saving the webarchive?

I wasn't using Safari. Safari doesn't archive the frame/data source, but instead the current DOM. This has its own set of issues which I am trying to avoid. [1] :-)

I'll attach a sample which demonstrates the problem the patch seeks to solve. This was testing against trunk builds of WebKit.

[1] Safari issue logged as rdar://problem/4961915

[Jim Correia]

Created attachment 12758 [details]
testcase

[David Kilzer (:ddkilzer)]

Comment on attachment 12754 [details]
Test (will crash ToT!)

Test case for Bug 12467.

[David Kilzer (:ddkilzer)]

Comment on attachment 12756 [details]
Stack trace

Stack trace for Bug 12467.

[David Kilzer (:ddkilzer)]

Removing Regression keyword since I'm not sure if this is a regression.  (Jim, does this crash with the shipping WebKit framework on Tiger?).

It's a crasher, so it's still P1.

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Removing Regression keyword since I'm not sure if this is a regression.  (Jim,
> does this crash with the shipping WebKit framework on Tiger?).
> 
> It's a crasher, so it's still P1.

Jim, is the test application supposed to crash?  Or does it simply not write a file when it fails?

[Jim Correia]

Created attachment 12764 [details]
testcase

[Jim Correia]

> Jim, is the test application supposed to crash?  Or does it simply not write a
> file when it fails?

The failure case is that a file isn't written because generating the WebArchive fails due to an exception. You should see this logged in the console:

2007-01-29 18:59:25.100 WebArchiveTester[15984] *** -[NSCFArray addObject:]: attempt to insert nil

I had a stupid bug in the last testcase app, so I've attached the fixed version which shows the prob.em.

The same problem happens with shipping WebKit on Tiger, but for a similar reason in a different piece of code (since the code has been significantly reworked since the Tiger snapshot.)

[Jim Correia]

Created attachment 12771 [details]
revised patch

The revised patch corrects brace formatting and includes a changelog.

[Jim Correia]

Created attachment 12773 [details]
revised patch (no tabs this time)

[Darin Adler]

Comment on attachment 12773 [details]
revised patch (no tabs this time)

Looks fine, but don't we need the same change in this method?

+ _archiveWithMarkupString:fromFrame:nodes:

I'd like to see this test added to the regression tests somehow. I think that can be done by enhancing DumpRenderTree.

[Jim Correia]

Created attachment 12789 [details]
patch

Revised patch with additional fix as suggested by darin.

[Darin Adler]

Comment on attachment 12789 [details]
patch

r=me

I'd much prefer this with a test in our regression tests. I think there *is* a way to do this with Dave Kilzer's new work.

[David Kilzer (:ddkilzer)]

(In reply to comment #21)
> I'd much prefer this with a test in our regression tests. I think there *is* a
> way to do this with Dave Kilzer's new work.

According to Jim, Safari doesn't use this code path to create webarchive files, so this would have to be an ObjC test.

[David Kilzer (:ddkilzer)]

(In reply to comment #22)
> (In reply to comment #21)
> > I'd much prefer this with a test in our regression tests. I think there *is* a
> > way to do this with Dave Kilzer's new work.
> According to Jim, Safari doesn't use this code path to create webarchive files,
> so this would have to be an ObjC test.

Which is to say that Bug 11882 changes DumpRenderTree to create webarchive test output using the same method that Safari does (I think).  I suppose we could add a different code path to DRT, or add some kind of option to the layoutTestController.dumpAsWebArchive() method to use a different code path.  I'll look into it.

[Mark Rowe (bdash)]

<rdar://problem/4971217>

[David Kilzer (:ddkilzer)]

Comment on attachment 12789 [details]
patch

>-            [subframeArchives addObject:[self _archiveCurrentStateForFrame:childFrame]];
>+            WebArchive *childFrameArchive = [self archiveFrame:childFrame];
>+            if (childFrameArchive)
>+                [subframeArchives addObject:childFrameArchive];

This is wrong for two reasons:

1. The new code does not match the behavior of the old code.

2. This code doesn't exhibit the same problem that the archiveFrame: method does.  After the page is loaded, the iframe contains "about:blank" when the WebArchiver archives the page, so there will never be an attempt to add nil to an NSMutableArray.

Darin, I need to extend DRT to make this fix testable.  The current method name is dumpAsWebArchive(), but I need two methods, one that dumps the current DOM into a webarchive (which is the current behavior) and one that dumps the original source into a webarchive.

I'm thinking of:  dumpDOMAsWebArchive() and dumpSourceAsWebArchive() or dumpFrameAsWebArchive().  Any opinions?

[David Kilzer (:ddkilzer)]

Comment on attachment 12773 [details]
revised patch (no tabs this time)

Clearing darin's r+ on old patch.

[David Kilzer (:ddkilzer)]

Created attachment 13514 [details]
Patch v2

Addresses issues in Comment #19, Comment #21 and Comment #25.

DRT and LayoutTest changes made at 32,000 ft over the western U.S. with my MBP in a "V" shape on my lap because the person in front of me reclined his seatback.  (Reclining mine didn't help as I was just in front of the exit row.  So close to real leg room, and yet, so far away.)

[Darin Adler]

Comment on attachment 13514 [details]
Patch v2

r=me

[David Kilzer (:ddkilzer)]

Committed revision 20018.