Bug 7293 – REGRESSION: Using Javascript Bookmarklets that reference location.href on a blank tab crashes WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jacob Lukas]

To reproduce:
1. Open a new tab.
2. Type javascript:void(location.href='http://www.google.com')
3. Press return. WebKit should almost immediately exit.

[Jacob Lukas]

Created attachment 6525 [details]
Crash Report

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Severity -> critical for crashers.

[Maciej Stachowiak]

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.

*** Bug 7403 has been marked as a duplicate of this bug. ***

Created attachment 6729 [details]
Patch

This fixes the crash, but I'm pretty sure it's not the right fix.

[Geoffrey Garen]

*** Bug 7428 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

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

[Darin Adler]

Comment on attachment 6743 [details]
Patch, including test and changelog

Looks fine. r=me

[Darin Adler]

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

[Geoffrey Garen]

Landed.