Bug 34008 - Assertion failure in KURL::setProtocol when running DOM Fuzzer
Summary: Assertion failure in KURL::setProtocol when running DOM Fuzzer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks: 29692
  Show dependency treegraph
 
Reported: 2010-01-22 10:41 PST by Alexey Proskuryakov
Modified: 2010-01-22 14:12 PST (History)
5 users (show)

See Also:


Attachments
proposed fix (10.62 KB, patch)
2010-01-22 10:51 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-01-22 10:41:40 PST
Patch forthcoming. This is harmless in release builds.
Comment 1 Alexey Proskuryakov 2010-01-22 10:51:31 PST
Created attachment 47212 [details]
proposed fix

We have similar issues with other KURL setters, but my primary goal here is to prevent the assertion failure when running DOM Fuzzer. Also, establishing correct behavior for each setter is not trivial, and is well worth a separate patch for each.
Comment 2 WebKit Review Bot 2010-01-22 11:05:46 PST
Attachment 47212 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/206004
Comment 3 Darin Adler 2010-01-22 11:40:33 PST
Google has their own KURL -- I wish that were not so -- so I guess someone has to fix that.
Comment 4 Alexey Proskuryakov 2010-01-22 11:52:15 PST
I can change KURLGoogle:setProtocol() to always return true, but Chromium would still have regressions in tests. Do we have the same "feel free to break it" agreement about KURLGoogle that we have about v8 bindings?
Comment 5 David Levin 2010-01-22 11:55:03 PST
(In reply to comment #4)
> I can change KURLGoogle:setProtocol() to always return true, but Chromium would
> still have regressions in tests. Do we have the same "feel free to break it"
> agreement about KURLGoogle that we have about v8 bindings?

I think so :(
I've cc'ed Albert who gets to handle this today.
Comment 6 Darin Adler 2010-01-22 12:17:09 PST
Comment on attachment 47212 [details]
proposed fix

It seems strange to move the "ignore everything after the first colon" behavior inside KURL::setProtocol. On the other hand, since it needs to be shared with location.protocol, I guess that was the handiest place to put it.

In HTMLAnchorElement::setProtocol you could do an early return if setProtocol returns false. This would avoid the question of why you can ignore the return value here, and it would slightly improve speed in the error case. But I suppose it's not all that great.

I think that at some point should rename from protocol to scheme in the URL class itself and related functions such as protocol/schemeIsJavaScript. Even if the DOM API calls it protocol, the URL specifications call it scheme and I think that's a clearer term.

invalid-protocol.js seems to have a UTF-8 BOM at the top of the file. I think a better way to do this would be to make the script-tests wrapper specify UTF-8 for the test script or use code to make the strings instead of using literal strings. But if you think we need to stick with the BOM then I think you should add a comment to the file letting people know the BOM is there and should not be removed.

r=me assuming you make a cut at writing the Google-specific code too or at least notify someone about fixing the Chromium build
Comment 7 Alexey Proskuryakov 2010-01-22 13:01:22 PST
Committed <http://trac.webkit.org/changeset/53712>. I made KURLGoogle.cpp changes that will hopefully prevent build failure, but not test failures.
Comment 8 Alexey Proskuryakov 2010-01-22 14:12:41 PST
Sorry, forgot to do anything about the UTF-8 BOM. But this isn't the first script test that includes one.