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.
Created attachment 12750 [details] Test Case described in bug body.
Created attachment 12751 [details] Patch/fix.
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).
Created attachment 12753 [details] changelog
Created attachment 12754 [details] Test (will crash ToT!)
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.
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?
(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?
(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
Created attachment 12758 [details] testcase
Comment on attachment 12754 [details] Test (will crash ToT!) Test case for Bug 12467.
Comment on attachment 12756 [details] Stack trace Stack trace for Bug 12467.
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.
(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?
Created attachment 12764 [details] testcase
> 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.)
Created attachment 12771 [details] revised patch The revised patch corrects brace formatting and includes a changelog.
Created attachment 12773 [details] revised patch (no tabs this time)
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.
Created attachment 12789 [details] patch Revised patch with additional fix as suggested by darin.
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.
(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.
(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.
<rdar://problem/4971217>
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 on attachment 12773 [details] revised patch (no tabs this time) Clearing darin's r+ on old patch.
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 on attachment 13514 [details] Patch v2 r=me
Committed revision 20018.