Summary: | REGRESSION: Using Javascript Bookmarklets that reference location.href on a blank tab crashes WebKit | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jacob Lukas <jlukas> | ||||||||
Component: | DOM | Assignee: | Maciej Stachowiak <mjs> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Critical | CC: | ap, ggaren, gibbonsb, mitz | ||||||||
Priority: | P1 | Keywords: | HasReduction, Regression | ||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
Attachments: |
|
Description
Jacob Lukas
2006-02-15 20:15:34 PST
Created attachment 6525 [details]
Crash Report
Confirmed. Crashers and regressions are P1. Added appropriate keywords. Works fine on Safari 2.0.3 (417.8) on 10.4.5. Crashes on locally-built ToT WebKit r12813 (anonsvn). Severity -> critical for crashers. I think I probably caused this w/ recent refactoring work. This is indeed a regression from r12811 (Improvements to frame loading). The crash happens because the frame exists but document() is nil here: void Location::put(ExecState *exec, const Identifier &p, JSValue *v, int attr) [...] Frame* p = Window::retrieveActive(exec)->frame(); if ( p ) url = p->document()->completeURL( str ); According to Maciej on IRC, a document used to be created as necessary before reaching here. The ultimate reason why this doesn't happen after r12811 is that previously -[WebCoreFrameBridge openURL:reload:contentType:refresh:lastModified:pageCache:] was called with 'about:blank' in this case, and now it is called with an empty NSURL. (The call site used to be -[WebDataSource(WebPrivate) _commitIfReady:] and now it's coming from -[WebFrame(WebPrivate) _commitProvisionalLoad:]). There seems to be another code path that creates a document on-demand, since entering javascript:document in the address bar of a newly-opened window works (and instantiates an HTMLDocumentImpl), and I think that's the code path that should kick in in the case of this bug as well. Having said that, another side-effect of the current state of things is that if you enter javascript: URLs in the address bar of a newly opened window, it will now erase the address bar after each URL, whereas previously it only did so after the first one (when there was no document). I believe that there is already a radar on the annoying first-time behavior. Created attachment 6729 [details]
Patch
This fixes the crash, but I'm pretty sure it's not the right fix.
*** Bug 7428 has been marked as a duplicate of this bug. *** Bug 7428 has a reduction that could easily turn into a layout test, although we should write another test for the javascript: case if we can. Comment on attachment 6729 [details] Patch I'm going to post a version with a layout test based on bug 7428 and a change log entry. Created attachment 6743 [details]
Patch, including test and changelog
Comment on attachment 6743 [details]
Patch, including test and changelog
Looks fine. r=me
(In reply to comment #7) > This fixes the crash, but I'm pretty sure it's not the right fix. I'd love to find a cleaner fix for this, but it's best to get this fix in and the layout test that checks the problematic case. We can always land a different fix later. Landed. |