WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
34492
avoid using invalidated KURL object in Element::baseURI
https://bugs.webkit.org/show_bug.cgi?id=34492
Summary
avoid using invalidated KURL object in Element::baseURI
Bryan Yeung
Reported
2010-02-02 10:45:52 PST
Currently, an invalidated KURL object is used to carry around a relative URL string in Element::baseURI. KURLGoogle does not support this behaviour, and I think it is better to not make use of invalidated KURL objects. Here is a simple patch that makes no semantic change and circumvents the use of the invalidated object.
Attachments
patch
(733 bytes, patch)
2010-02-02 11:00 PST
,
Bryan Yeung
darin
: review-
Details
Formatted Diff
Diff
revised patch
(1.47 KB, patch)
2010-02-02 11:22 PST
,
Bryan Yeung
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2010-02-02 10:53:05 PST
(In reply to
comment #0
)
> Here is a simple patch that makes no semantic change and circumvents the use of > the invalidated object.
Where?
Darin Adler
Comment 2
2010-02-02 10:57:53 PST
Please be clear, holding arbitrary strings that are not valid URLs is an intentional feature of KURL. To remove it we need to remove all clients that depend on that. It sure would be great to have the freedom to not need that feature, but we can’t get that by just fixing the cases we have already stumbled on, we need to look at all callers. And then change the implementation so the mistake is easy to detect. I know that Chromium has been living with all these bugs all along since KURLGoogle omitted that feature, but when fixing this please keep in mind that all other platforms have not been subject to the bugs. KURLGoogle still seems like a bad idea to me. I know it’s pleasant for Chromium to share the same URL parsing with the other components in Chromium -- it would be nice to have that on the Mac too -- and someone may have the idea that it helps with security, but I believe that for the WebKit project as a whole would be great to make changes as needed to both the original KURL and KURLGoogle so their behavior is identical and then get rid of this unnecessary difference between platforms by deleting KURLGoogle.
Bryan Yeung
Comment 3
2010-02-02 11:00:12 PST
Created
attachment 47952
[details]
patch Does it not work to add a patch from the bug creation page? Anyway, here is the patch.
Bryan Yeung
Comment 4
2010-02-02 11:00:46 PST
Sorry! Wrong bug to attach the patch to. Please ignore.
Bryan Yeung
Comment 5
2010-02-02 11:01:52 PST
(In reply to
comment #4
)
> Sorry! Wrong bug to attach the patch to. Please ignore.
Nope: I just didn't realize there were more comments, so this looked like a different bug. Clearly I'm not really used to bugzilla. Sorry for the confusion and extraneous comments.
Darin Adler
Comment 6
2010-02-02 11:02:38 PST
(In reply to
comment #3
)
> Does it not work to add a patch from the bug creation page?
I never tried it before. Maybe it’s broken.
Darin Adler
Comment 7
2010-02-02 11:05:24 PST
Comment on
attachment 47952
[details]
patch Change looks great, although I’d probably use assignment syntax rather than initialization to set up the baseAttribute local variable. Generally it's not correct to set the review flag on a patch without a change log entry. We require regression tests for bugs fixes. Since this patch fixes a bug, you must also include a regression test that fails on Chromium before this change and now succeeds with the fix. But neither the test itself nor the test results should be Chromium-specific. review- because of the lack of change log and regression test
Bryan Yeung
Comment 8
2010-02-02 11:22:33 PST
Created
attachment 47955
[details]
revised patch Sorry for the incomplete patch last time. This includes the ChangeLog entry. There was already a test failing on Chromium without this change (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an additional regression test is necessary.
Darin Adler
Comment 9
2010-02-02 11:25:40 PST
(In reply to
comment #8
)
> Sorry for the incomplete patch last time. This includes the ChangeLog entry. > There was already a test failing on Chromium without this change > (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an > additional regression test is necessary.
Typically a patch would include the corrected results or a change to the skipped list for a patch -- so I could see what test was changing behavior. But since Chromium has a different way of doing things, maybe that's why there's no such change here. Despite the fact that a test was failing, a simpler test that targets this behavior more directly would be far better. It could even be a dumpAsText test. Tests where success can only be judged from the pixel results are the lowest on our totem pole of test usefulness, so this existing test is only barely sufficient.
Darin Adler
Comment 10
2010-02-02 11:26:32 PST
Comment on
attachment 47955
[details]
revised patch
> +2010-02-02 Bryan Yeung <bryeung@bryeung-chrome.(none)>
Strange address here. r=me despite my concerns mentioned in comments above.
WebKit Commit Bot
Comment 11
2010-02-02 12:04:36 PST
Comment on
attachment 47955
[details]
revised patch Clearing flags on attachment: 47955 Committed
r54245
: <
http://trac.webkit.org/changeset/54245
>
WebKit Commit Bot
Comment 12
2010-02-02 12:04:44 PST
All reviewed patches have been landed. Closing bug.
David Levin
Comment 13
2010-02-02 16:54:23 PST
Committed
r54264
: <
http://trac.webkit.org/changeset/54264
>
David Levin
Comment 14
2010-02-02 17:06:05 PST
Rolled out due to major chromium bustage with this patch. Rolllout:
http://trac.webkit.org/changeset/54264
David Levin
Comment 15
2010-02-02 17:54:23 PST
Actually, it looks like I may have been wrong. Once the canary bot gets well. I'd be happy to re-commit this (since it was rolled out incorrectly) -- sometimes it is hard to tell which patch caused the issue :(
David Levin
Comment 16
2010-02-03 08:11:15 PST
Committed again unchanged from before
http://trac.webkit.org/changeset/54282
Sorry about the mix-up here.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug