Bug 66874 – Get rid of frame life support timer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

Postponing frame deletion on a zero delay timer makes behavior less predictable for both WebKit developers and JS developers. Frame lifetime shouldn't depend on JS execution.

Perhaps surprisingly, very few regression tests depend on the life support timer. There is a risk of web site regressions, but we'll see.

[Geoffrey Garen]

I feel the same giddy fear that a 5-year-old might feel before launching a firecracker.

[Alexey Proskuryakov]

Created attachment 105036 [details]
proposed patch

I'm surprised that I didn't need to add RefPtr protectors anywhere. My theory is that the keep alive timer wasn't working in some situations, so we had to fix all such issues over time already.

[WebKit Review Bot]

Comment on attachment 105036 [details]
proposed patch

Attachment 105036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9488212

[Geoffrey Garen]

Source/WebCore/bindings/v8/ScriptController.cpp: In member function 'WebCore::ScriptValue WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode&)':
Source/WebCore/bindings/v8/ScriptController.cpp:194: error: 'class WebCore::Frame' has no member named 'keepAlive'
make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/bindings/v8/ScriptController.o] Error 1

Ah, the v8 bindings. An epic contribution to the WebKit project.

[Alexey Proskuryakov]

Created attachment 105044 [details]
with v8 build fixes

[Geoffrey Garen]

> Source/WebCore/bindings/js/JSDOMWindowBase.h:-58
> -        virtual JSC::ExecState* globalExec();

You can make globalExec() in JavaScriptCore non-virtual now. The only reason it's virtual is to allow for this override.

[Alexey Proskuryakov]

Created attachment 105046 [details]
with devirtualized globalExec()

Looks like I can!

[Dmitry Titov]

I have run Layout tests on Chromium try bot with this patch applied and got following results: http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/1362
It seems there are some crashes in tests that may be relevant. The crash stacks are in 'stdio' file of the "webkit tests' step.

Just curious, some patches in the past did not have layout tests, is it possible to verify what the issues they were fixing are not regressing? This is one example: http://trac.webkit.org/changeset/7546

Also, some changes in the past do not have information about them, like this one: http://trac.webkit.org/changeset/42417. Do we know what was fixed by them?

[WebKit Review Bot]

Comment on attachment 105046 [details]
with devirtualized globalExec()

Attachment 105046 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9510030

New failing tests:
http/tests/fileapi/create-blob-url-from-data-url.html
http/tests/websocket/tests/hybi/close-on-unload-reference-in-parent.html
fast/frames/iframe-scale-applied-twice.html
http/tests/websocket/tests/hybi/send-after-close-on-unload.html
http/tests/websocket/tests/hixie76/close-on-unload.html
http/tests/websocket/tests/hixie76/close-on-unload-reference-in-parent.html
fast/replaced/frame-removed-during-resize.html
http/tests/websocket/tests/hybi/close-on-unload.html
fast/dom/xmlhttprequest-constructor-in-detached-document.html
http/tests/websocket/tests/hixie76/send-after-close-on-unload.html

[Darin Adler]

(In reply to comment #8)
> Just curious, some patches in the past did not have layout tests, is it possible to verify what the issues they were fixing are not regressing? This is one example: http://trac.webkit.org/changeset/7546

I can attach the test case from that bug.

[Darin Adler]

Created attachment 105090 [details]
Test case for the bug fixed in r7546

[Alexey Proskuryakov]

Created attachment 105096 [details]
trying to fix v8 tests

The test for r7546 passes for me with this patch.

[Geoffrey Garen]

Comment on attachment 105096 [details]
trying to fix v8 tests

r=me, assuming tests pass.

[WebKit Review Bot]

Comment on attachment 105096 [details]
trying to fix v8 tests

Attachment 105096 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9513125

New failing tests:
http/tests/fileapi/create-blob-url-from-data-url.html
http/tests/websocket/tests/hybi/close-on-unload-reference-in-parent.html
fast/frames/iframe-scale-applied-twice.html
http/tests/websocket/tests/hybi/send-after-close-on-unload.html
http/tests/websocket/tests/hixie76/close-on-unload.html
http/tests/websocket/tests/hixie76/close-on-unload-reference-in-parent.html
fast/replaced/frame-removed-during-resize.html
http/tests/websocket/tests/hybi/close-on-unload.html
fast/dom/xmlhttprequest-constructor-in-detached-document.html
http/tests/websocket/tests/hixie76/send-after-close-on-unload.html

[Alexey Proskuryakov]

So, my attempt to fix v8 bindings didn't work. Not sure how to proceed - is the agreement that we don't need to care about v8 bindings still in force? I'd certainly prefer to avoid introducing regressions, but I cannot fix v8 myself.

[Adam Barth]

Can I help?  /me looks at the patch.

[Adam Barth]

Is this code needed to make Magic iframes work?  I never fully understood how those worked.

This looks a bit complicated to figure out at 1am, but I can take a look tomorrow in more detail.  If you'd be willing to wait a day before landing this patch, hopefully we can land it in a clean way.

[Darin Adler]

I don’t even know what “magic iframes” are. I’d love to learn.

[Alexey Proskuryakov]

I think that it's about transferring live subframes between documents. I'm also suspicious of this case, although regression tests seem to pass. Perhaps Dmitry knows why these aren't affected.

I'm certainly willing to wait a day or even longer than that. Your help is very appreciated!

[Adam Barth]

> I don’t even know what “magic iframes” are. I’d love to learn.

As Alexey says, it allows a web site to move an iframe from one document to another without causing it to reload (much like you can move a <video> element from one page to another without disrupting playback).  The idea came out of the discussion about global pages (or whatever it was called).  It's used, for example, to make the chat moles in Gmail pop out quickly.

[Darin Adler]

I knew about the feature, but didn’t know about the “magic” name.

[Alexey Proskuryakov]

Created attachment 105214 [details]
plus windows build fix, minus failed v8 fix

[Dmitry Titov]

I will take a look at v8 issue later today if Adam won't beat me to it.

One of GMail engineers named the feature of transferring live iframe the "magic iframe" and it somehow stuck around :-)

[WebKit Review Bot]

Comment on attachment 105214 [details]
plus windows build fix, minus failed v8 fix

Attachment 105214 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9495607

New failing tests:
http/tests/fileapi/create-blob-url-from-data-url.html
http/tests/websocket/tests/hybi/close-on-unload-reference-in-parent.html
fast/frames/iframe-scale-applied-twice.html
http/tests/websocket/tests/hybi/send-after-close-on-unload.html
http/tests/websocket/tests/hixie76/close-on-unload.html
http/tests/websocket/tests/hixie76/close-on-unload-reference-in-parent.html
fast/replaced/frame-removed-during-resize.html
http/tests/websocket/tests/hybi/close-on-unload.html
fast/dom/xmlhttprequest-constructor-in-detached-document.html
http/tests/websocket/tests/hixie76/send-after-close-on-unload.html

[Dmitry Titov]

Created attachment 105289 [details]
Lets see if it fixes v8 crashes

It's the latest Alexey's patch, with 2 protection pointes added in V8Proxy.cpp. Lets see what chromium EWS says.

[Dmitry Titov]

Created attachment 105290 [details]
Same, with style fix.

[WebKit Review Bot]

Comment on attachment 105290 [details]
Same, with style fix.

Attachment 105290 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9510777

New failing tests:
fast/dom/xmlhttprequest-constructor-in-detached-document.html

[Adam Barth]

That seems better.

[Dmitry Titov]

I bet it needs another RefPtr somewhere.. Stay tuned.

[Alexey Proskuryakov]

This test has some JSC bindings specific output, so it might just need custom results for v8. Fantastic!

The patch also needs more Windows build fixing...

[Dmitry Titov]

Indeed, the remaining v8 test is failing because it needs v8-specific result (in LayoutTests/platform/chromium). Here is the diff:

-CONSOLE MESSAGE: line 12: ReferenceError: XMLHttpRequest constructor associated document is unavailable
+CONSOLE MESSAGE: line 12: Uncaught ReferenceError: XMLHttpRequest constructor's associated context is not available

You may leave it as is, Chromium WebKit gardener (Dave Levin for tomorrow) will be able to rebaseline it. Added him to Cc.

[Alexey Proskuryakov]

Created attachment 105389 [details]
patch for landing

Thank you!

Fixed Windows build and added custom results for v8.

[WebKit Review Bot]

Comment on attachment 105389 [details]
patch for landing

Rejecting attachment 105389 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
tructor-in-detached-document.html = TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected image and text mismatch : (1)
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT



Full output: http://queues.webkit.org/results/9541170

[WebKit Review Bot]

Attachment 105389 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

LayoutTests/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

(In reply to comment #34)
> Attachment 105389 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> 
> LayoutTests/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
> Total errors found: 1 in 13 files

Not a very descriptive error but there seems to be a missing colon on that line. (Not worth fixing the patch  imo.)

[Dmitry Titov]

The test results look incorrect/unrelated. I run the tests in specified directories locally with the patch and it's all right.

The EWS may barf because the fast/dom/xmlhttprequest-constructor-in-detached-document.html is still failing. It will still fail because there are os-specific versions of the results captured that have to be removed in following dirs:
LayoutTests/platform/chromium-cg-mac
LayoutTests/platform/chromium-win

In presence of more-specific results they will be used first, that's why compare fails.

[Alexey Proskuryakov]

I see. It's surprising that this test has custom results on some platforms for Chromium. Hopefully a common result will work now for all of them (but sadly, not the main cross-platform result).

Landed manually in <http://trac.webkit.org/changeset/93913> without Chromium test fixes, Dave is going to take care of these.

[Dmitry Titov]

(In reply to comment #37)
> I see. It's surprising that this test has custom results on some platforms for Chromium.

They are identical results. The rebaselining tool that Chromium Webkit gardeners use at some point was not very particular about coalescing results across platforms. Also gardener can grab new snapshots from canary servers for particular platform while other servers did not yet run the test and so they don't have a snapshot that could be coalesced. We likely do have some duplication of results in platform/chromium because of various reasons.

[David Levin]

(In reply to comment #38)
> (In reply to comment #37)
> > I see. It's surprising that this test has custom results on some platforms for Chromium.
> 
> They are identical results. The rebaselining tool that Chromium Webkit gardeners use at some point was not very particular about coalescing results across platforms. Also gardener can grab new snapshots from canary servers for particular platform while other servers did not yet run the test and so they don't have a snapshot that could be coalesced. We likely do have some duplication of results in platform/chromium because of various reasons.

The new tool does a good job of coalescing the results and when I rebaselined this, it got fixed up.

Disappointing that we have to have a different result at all (but at the moment I have no time to pursue that -- maybe in the distant future).