Bug 12463 - WebArchiver - attempt to insert nil exception when archive empty iframe
Summary: WebArchiver - attempt to insert nil exception when archive empty iframe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2007-01-29 12:02 PST by Jim Correia
Modified: 2007-03-07 07:14 PST (History)
2 users (show)

See Also:


Attachments
Test Case described in bug body. (358 bytes, text/html)
2007-01-29 12:02 PST, Jim Correia
no flags Details
Patch/fix. (809 bytes, patch)
2007-01-29 12:03 PST, Jim Correia
no flags Details | Formatted Diff | Diff
changelog (207 bytes, text/plain)
2007-01-29 12:28 PST, Jim Correia
no flags Details
Test (will crash ToT!) (838 bytes, application/x-webarchive)
2007-01-29 12:30 PST, David Kilzer (:ddkilzer)
no flags Details
Stack trace (2.01 KB, text/plain)
2007-01-29 12:34 PST, David Kilzer (:ddkilzer)
no flags Details
testcase (26.64 KB, application/zip)
2007-01-29 14:13 PST, Jim Correia
no flags Details
testcase (26.67 KB, application/zip)
2007-01-29 16:08 PST, Jim Correia
no flags Details
revised patch (1.37 KB, patch)
2007-01-29 20:42 PST, Jim Correia
no flags Details | Formatted Diff | Diff
revised patch (no tabs this time) (1.37 KB, patch)
2007-01-29 21:01 PST, Jim Correia
no flags Details | Formatted Diff | Diff
patch (2.09 KB, patch)
2007-01-30 05:42 PST, Jim Correia
ddkilzer: review-
Details | Formatted Diff | Diff
Patch v2 (26.05 KB, patch)
2007-03-07 04:06 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jim Correia 2007-01-29 12:02:13 PST
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.
Comment 1 Jim Correia 2007-01-29 12:02:56 PST
Created attachment 12750 [details]
Test Case described in bug body.
Comment 2 Jim Correia 2007-01-29 12:03:34 PST
Created attachment 12751 [details]
Patch/fix.
Comment 3 David Kilzer (:ddkilzer) 2007-01-29 12:27:59 PST
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).

Comment 4 Jim Correia 2007-01-29 12:28:48 PST
Created attachment 12753 [details]
changelog
Comment 5 David Kilzer (:ddkilzer) 2007-01-29 12:30:07 PST
Created attachment 12754 [details]
Test (will crash ToT!)
Comment 6 David Kilzer (:ddkilzer) 2007-01-29 12:34:01 PST
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.
Comment 7 Jim Correia 2007-01-29 13:01:55 PST
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?
Comment 8 David Kilzer (:ddkilzer) 2007-01-29 13:21:49 PST
(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?
Comment 9 Jim Correia 2007-01-29 14:12:39 PST
(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
Comment 10 Jim Correia 2007-01-29 14:13:28 PST
Created attachment 12758 [details]
testcase
Comment 11 David Kilzer (:ddkilzer) 2007-01-29 15:09:07 PST
Comment on attachment 12754 [details]
Test (will crash ToT!)

Test case for Bug 12467.
Comment 12 David Kilzer (:ddkilzer) 2007-01-29 15:09:48 PST
Comment on attachment 12756 [details]
Stack trace

Stack trace for Bug 12467.
Comment 13 David Kilzer (:ddkilzer) 2007-01-29 15:13:08 PST
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.

Comment 14 David Kilzer (:ddkilzer) 2007-01-29 15:31:31 PST
(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?
Comment 15 Jim Correia 2007-01-29 16:08:42 PST
Created attachment 12764 [details]
testcase
Comment 16 Jim Correia 2007-01-29 16:11:06 PST
> 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.)
Comment 17 Jim Correia 2007-01-29 20:42:49 PST
Created attachment 12771 [details]
revised patch

The revised patch corrects brace formatting and includes a changelog.
Comment 18 Jim Correia 2007-01-29 21:01:07 PST
Created attachment 12773 [details]
revised patch (no tabs this time)
Comment 19 Darin Adler 2007-01-29 22:14:27 PST
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.
Comment 20 Jim Correia 2007-01-30 05:42:37 PST
Created attachment 12789 [details]
patch

Revised patch with additional fix as suggested by darin.
Comment 21 Darin Adler 2007-01-30 09:49:26 PST
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.
Comment 22 David Kilzer (:ddkilzer) 2007-01-30 10:05:32 PST
(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.

Comment 23 David Kilzer (:ddkilzer) 2007-01-30 10:10:58 PST
(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.

Comment 24 Mark Rowe (bdash) 2007-02-01 18:41:14 PST
<rdar://problem/4971217>
Comment 25 David Kilzer (:ddkilzer) 2007-02-04 22:27:56 PST
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?
Comment 26 David Kilzer (:ddkilzer) 2007-02-04 22:29:44 PST
Comment on attachment 12773 [details]
revised patch (no tabs this time)

Clearing darin's r+ on old patch.
Comment 27 David Kilzer (:ddkilzer) 2007-03-07 04:06:55 PST
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.)
Comment 28 Darin Adler 2007-03-07 06:25:22 PST
Comment on attachment 13514 [details]
Patch v2

r=me
Comment 29 David Kilzer (:ddkilzer) 2007-03-07 07:14:49 PST
Committed revision 20018.