Bug 221375 – WebKitTestRunner can get stuck stuck on onload = () => document.createElement('embed');

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Ryosuke Niwa]

WebKitTestRunner get stuck on this:

<script>
  onload = () => document.createElement('embed');
</script>

the results are the same for the object and applet tags.

Steps To Reproduce:

DYLD_FRAMEWORK_PATH=<WEBKIT_PATH>/WebKitBuild/Debug <WEBKIT_PATH>/WebKitBuild/Debug/WebKitTestRunner repro_407.html

Results:

WebKitTestRunner gets stuck, waiting forever.

For the slightly modified test case:

<script>
  onload = () => {
      testRunner.waitUntilDone();
      testRunner.notifyDone();
      $vm.print('before');
      document.createElement('embed');
      $vm.print('after');
      throw new Error(`an error`);
    }
</script>

<rdar://problem/69554695>

[Rob Buis]

Created attachment 419592 [details]
Patch

[Alex Christensen]

Comment on attachment 419592 [details]
Patch

Definitely needs a test.
I don't think this is the right fix, because if it were we would need to do this at all call sites of incrementLoadEventDelayCount and we don't.

[Rob Buis]

Created attachment 419728 [details]
Patch

[Rob Buis]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 419592 [details]
> Patch
> 
> Definitely needs a test.
> I don't think this is the right fix, because if it were we would need to do
> this at all call sites of incrementLoadEventDelayCount and we don't.

No test at the time since I was not sure if it is a security issue. I am now more convinced it is not a security issue, so I added a test in the new patch.

[Alex Christensen]

Comment on attachment 419728 [details]
Patch

This isn't obviously wrong like the first one was, but I don't know why finishCreating was there in the first place, or what the consequences of this can be.

[Rob Buis]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 419728 [details]
> Patch
> 
> This isn't obviously wrong like the first one was, but I don't know why
> finishCreating was there in the first place, or what the consequences of
> this can be.

The relevant change seems to be https://bugs.webkit.org/show_bug.cgi?id=130653.
AFAICS that change moved scheduling to an earlier point in time, i.e. element creation instead of children parsing to be finished or src to be set. Not sure why that change in behaviour (if I have read that code right) would be needed, maybe Darin remembers? The scheduling is done even though the embed element may never be appended to the document, like in the test case in this bug.

[Ryosuke Niwa]

Comment on attachment 419728 [details]
Patch

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

This looks right. We call scheduleUpdateForAfterStyleResolution() whenever we parse an attribute so it should be safe to remove the call in the constructor.

> LayoutTests/plugins/embed-creation-crash-expected.txt:1
> +This test should not crash.

You mean this shouldn't hang?

[Rob Buis]

Comment on attachment 419728 [details]
Patch

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

>> LayoutTests/plugins/embed-creation-crash-expected.txt:1
>> +This test should not crash.
> 
> You mean this shouldn't hang?

Good point, will fix before landing.

[Rob Buis]

Created attachment 419818 [details]
Patch

[EWS]

Committed r272640: <https://commits.webkit.org/r272640>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419818 [details].

[Ryosuke Niwa]

(In reply to Rob Buis from comment #8)
> Comment on attachment 419728 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419728&action=review
> 
> >> LayoutTests/plugins/embed-creation-crash-expected.txt:1
> >> +This test should not crash.
> > 
> > You mean this shouldn't hang?
> 
> Good point, will fix before landing.

You forgot to rename the file name LOL.
rs=me on the rename from -crash to -hang.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 419846 [details]
Patch

[Rob Buis]

Created attachment 419852 [details]
Patch

[EWS]

Committed r272676: <https://commits.webkit.org/r272676>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419852 [details].