Changeset 41484 in webkit


Ignore:
Timestamp:
Mar 6, 2009, 9:22:07 AM (16 years ago)
Author:
Darin Adler
Message:

WebCore:

2009-03-06 Darin Adler <Darin Adler>

Reviewed by Darin Fisher.

Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
​https://bugs.webkit.org/show_bug.cgi?id=24422
rdar://problem/6402208

Test: fast/dom/location-new-window-no-crash.html

The issue here is empty (or null) URLs. I picked the "schedule navigation" bottleneck
to add some checks for empty URLs. We could also put the empty URL checks at some
other bottleneck level and add more assertions over time. I tried adding a few more
assertions to functions like loadURL and hit them while running the regression tests,
so it's probably going to be a bit tricky to clean this up throughout the loader.

  • loader/FrameLoader.cpp: (WebCore::ScheduledRedirection::ScheduledRedirection): Explicitly marked this struct immutable by making all its members const. Added assertions about the arguments, including that the URL is not empty. Initialized one uninitialized member in one of the constructors. (WebCore::FrameLoader::scheduleHTTPRedirection): Added an early exit to make this a no-op if passed an empty URL. (WebCore::FrameLoader::scheduleLocationChange): Ditto. (WebCore::FrameLoader::scheduleRefresh): Ditto.

LayoutTests:

2009-03-06 Darin Adler <Darin Adler>

Reviewed by Darin Fisher.

Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
​https://bugs.webkit.org/show_bug.cgi?id=24422
rdar://problem/6402208

The new test manipulates all the properties of the location object on a new window which
has no location yet. I tested Firefox too and added comments about how its behavior differs
from WebKit. At some point we may want to tweak our behavior to be a bit closer to theirs,
or check IE's behavior or if HTML 5 or some other W3 specification has something to say
about this, but for now the main purpose of the test is to verify we don't crash.

  • fast/dom/location-new-window-no-crash-expected.txt: Added.
  • fast/dom/location-new-window-no-crash.html: Added.
  • fast/dom/resources/location-new-window-no-crash.js: Added.
Location:
trunk
Files:
3 added
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r41480 r41484  
     12009-03-06  Darin Adler  <darin@apple.com>
     2
     3        Reviewed by Darin Fisher.
     4
     5        Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
     6        https://bugs.webkit.org/show_bug.cgi?id=24422
     7        rdar://problem/6402208
     8
     9        The new test manipulates all the properties of the location object on a new window which
     10        has no location yet. I tested Firefox too and added comments about how its behavior differs
     11        from WebKit. At some point we may want to tweak our behavior to be a bit closer to theirs,
     12        or check IE's behavior or if HTML 5 or some other W3 specification has something to say
     13        about this, but for now the main purpose of the test is to verify we don't crash.
     14
     15        * fast/dom/location-new-window-no-crash-expected.txt: Added.
     16        * fast/dom/location-new-window-no-crash.html: Added.
     17        * fast/dom/resources/location-new-window-no-crash.js: Added.
     18
    1192009-03-06  Darin Adler  <darin@apple.com>
    220

  • trunk/WebCore/ChangeLog

    r41483 r41484  
     12009-03-06  Darin Adler  <darin@apple.com>
     2
     3        Reviewed by Darin Fisher.
     4
     5        Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
     6        https://bugs.webkit.org/show_bug.cgi?id=24422
     7        rdar://problem/6402208
     8
     9        Test: fast/dom/location-new-window-no-crash.html
     10
     11        The issue here is empty (or null) URLs. I picked the "schedule navigation" bottleneck
     12        to add some checks for empty URLs. We could also put the empty URL checks at some
     13        other bottleneck level and add more assertions over time. I tried adding a few more
     14        assertions to functions like loadURL and hit them while running the regression tests,
     15        so it's probably going to be a bit tricky to clean this up throughout the loader.
     16
     17        * loader/FrameLoader.cpp:
     18        (WebCore::ScheduledRedirection::ScheduledRedirection): Explicitly marked this struct
     19        immutable by making all its members const. Added assertions about the arguments,
     20        including that the URL is not empty. Initialized one uninitialized member in one of
     21        the constructors.
     22        (WebCore::FrameLoader::scheduleHTTPRedirection): Added an early exit to make this
     23        a no-op if passed an empty URL.
     24        (WebCore::FrameLoader::scheduleLocationChange): Ditto.
     25        (WebCore::FrameLoader::scheduleRefresh): Ditto.
     26
    1272009-03-06  Gustavo Noronha Silva  <gns@gnome.org>
    228

  • trunk/WebCore/loader/FrameLoader.cpp

    r41431 r41484  
    145145struct ScheduledRedirection {
    146146    enum Type { redirection, locationChange, historyNavigation, locationChangeDuringLoad };
    147     Type type;
    148     double delay;
    149     String url;
    150     String referrer;
    151     int historySteps;
    152     bool lockHistory;
    153     bool lockBackForwardList;
    154     bool wasUserGesture;
    155     bool wasRefresh;
     147
     148    const Type type;
     149    const double delay;
     150    const String url;
     151    const String referrer;
     152    const int historySteps;
     153    const bool lockHistory;
     154    const bool lockBackForwardList;
     155    const bool wasUserGesture;
     156    const bool wasRefresh;
    156157
    157158    ScheduledRedirection(double delay, const String& url, bool lockHistory, bool lockBackForwardList, bool wasUserGesture, bool refresh)
    … …  
    165166        , wasRefresh(refresh)
    166167    {
     168        ASSERT(!url.isEmpty());
    167169    }
    168170
    … …  
    178180        , wasRefresh(refresh)
    179181    {
     182        ASSERT(locationChangeType == locationChange || locationChangeType == locationChangeDuringLoad);
     183        ASSERT(!url.isEmpty());
    180184    }
    181185
    … …  
    185189        , historySteps(historyNavigationSteps)
    186190        , lockHistory(false)
     191        , lockBackForwardList(false)
    187192        , wasUserGesture(false)
    188193        , wasRefresh(false)
    … …  
    373378}
    374379
    375 
    376380void FrameLoader::changeLocation(const KURL& url, const String& referrer, bool lockHistory, bool lockBackForwardList, bool userGesture, bool refresh)
    377381{
    … …  
    13141318        return;
    13151319
     1320    if (url.isEmpty())
     1321        return;
     1322
    13161323    // We want a new history item if the refresh timeout is > 1 second.
    13171324    if (!m_scheduledRedirection || delay <= m_scheduledRedirection->delay)
    … …  
    13221329{
    13231330    if (!m_frame->page())
     1331        return;
     1332
     1333    if (url.isEmpty())
    13241334        return;
    13251335
    … …  
    13531363{
    13541364    if (!m_frame->page())
     1365        return;
     1366
     1367    if (m_URL.isEmpty())
    13551368        return;
    13561369
    … …  
    14661479{
    14671480    ASSERT(childFrame);
     1481
    14681482    HistoryItem* parentItem = currentHistoryItem();
    14691483    FrameLoadType loadType = this->loadType();
    … …  
    14721486    KURL workingURL = url;
    14731487   
    1474     // If we're moving in the backforward list, we might want to replace the content
     1488    // If we're moving in the back/forward list, we might want to replace the content
    14751489    // of this child frame with whatever was there at that point.
    14761490    if (parentItem && parentItem->children().size() && isBackForwardLoadType(loadType)) {
Note: See TracChangeset for help on using the changeset viewer.