Bug 94058 – [BlackBerry] [DRT] 5 tests failed for incorrect JS console message format

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Xiaobo Wang]

From RIM PR #191441

Failed cases:
1 fast/frames/sandboxed-iframe-history-denied.html
2 fast/frames/sandboxed-iframe-navigation-targetlink.html
3 fast/frames/sandboxed-iframe-navigation-top-by-name-denied.html
4 fast/frames/sandboxed-iframe-navigation-top-denied.html
5 fast/frames/seamless/seamless-window-location-sandbox.html

All tests failed because of incorrect console message format.
Need to fix in DumpRenderTree code.
Here's the text diff for one of the tests.

---/tmp/developer/layout-test-results/fast/frames/sandboxed-iframe-navigation-top-denied-expected.txt
+++/tmp/developer/layout-test-results/fast/frames/sandboxed-iframe-navigation-top-denied-actual.txt
@@ -1,4 +1,6 @@
-CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate a navigation change for frame with URL navigate-top-to-fail.html.
+CONSOLE MESSAGE: line 0: Unsafe JavaScript attempt to initiate a navigation change for frame with URL file:///developer/LayoutTests/fast/frames/sandboxed-iframe-navigation-top-denied.html from frame with URL file:///developer/LayoutTests/fast/frames/resources/navigate-top-to-fail.html.
+
+JS Console: :0: Unsafe JavaScript attempt to initiate a navigation change for frame with URL file:///developer/LayoutTests/fast/frames/sandboxed-iframe-navigation-top-denied.html from frame with URL file:///developer/LayoutTests/fast/frames/resources/navigate-top-to-fail.html.

 This test verifies that a sandboxed IFrame cannot navigate the top-level frame without allow-top-navigation. This test passes if the navigation does not occur.

[Xiaobo Wang]

Created attachment 158486 [details]
patch

Patch for this issue. The Source part is patched by Rob Buis.

[Xiaobo Wang]

Adding Yong, Antonio, and George.
Can any of you help to review?

[Rob Buis]

Comment on attachment 158486 [details]
patch

Thanks for this patch, looks good!

[Yong Li]

Comment on attachment 158486 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158486&action=review

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:653
> +        String remaining = message.substring(pos, message.length() - pos);

", message.length() - pos" is not necessary

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:657
> +            baseName = remaining.substring(indexBase + 1, remaining.length() - indexBase - 1);

ditto. Also it should be called "fileName" rather than "baseName"? "Base" sounds like base URL / the folder path...

[Xiaobo Wang]

(In reply to comment #4)
> (From update of attachment 158486 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158486&action=review
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:653
> > +        String remaining = message.substring(pos, message.length() - pos);
> 
> ", message.length() - pos" is not necessary
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:657
> > +            baseName = remaining.substring(indexBase + 1, remaining.length() - indexBase - 1);
> 
> ditto. Also it should be called "fileName" rather than "baseName"? "Base" sounds like base URL / the folder path...

Good suggestions Yong. I agree "fileName" is better. Actually I borrowed the name from the UNIX command "basename" which usually returns the file name of a full path. Will update the patch soon.

[Xiaobo Wang]

Created attachment 158745 [details]
patch updated

Updated according to comments from Yong Li.

[Yong Li]

Comment on attachment 158745 [details]
patch updated

View in context: https://bugs.webkit.org/attachment.cgi?id=158745&action=review

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:662
> +    printf("%s\n", newMessage.utf8().data());

I have introduced a class ReadOnlyLatinString that will give better performance if you are sure latin1() won't lose any info and in most cases the String has a 8-bit internal buffer. But this is minor thing, especially this is only for DRT.

[WebKit Review Bot]

Comment on attachment 158745 [details]
patch updated

Clearing flags on attachment: 158745

Committed r125778: <http://trac.webkit.org/changeset/125778>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.