[e10s] resizeBy not working as expected immediately after popup opens

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: szmydadam, Assigned: mystor)

Tracking

53 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

9 months ago
Created attachment 8873722 [details]
Current and expected result

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

1. Open a popup i.e.: `var popup = window.open('', '', 'width=200,height=200');`
2. Right after that, run resizeBy method on that popup: `popup.resizeBy(50,0);`

Fiddle: https://jsfiddle.net/peo3x4aq/
Fiddle with working workaround (timeout): https://jsfiddle.net/peo3x4aq/1/


Actual results:

Popup gets shrink to very small size.


Expected results:

Popup should just be resized horizontally by 50px

Updated

9 months ago
Component: Untriaged → DOM
Product: Firefox → Core

Comment 1

9 months ago
It's an issue only with e10s enabled. FF40 has the bug, therefore the issue is probably here since the introduction of multiprocess.
Summary: resizeBy not working as expected immediately after popup opens → [e10s] resizeBy not working as expected immediately after popup opens
I noticed Boris' footprints on ResizeBy* code. Perhaps he knows where our code goes wrong in e10s case.
Flags: needinfo?(bzbarsky)
No idea offhand; I'd have to step through this.  Presumably the "get the current size" codepath doesn't get the right numbers in e10s mode or something, so we end up doing more like a resizeTo.

Michael, want to take a look?  You've been in this code a lot recently... ;)
Flags: needinfo?(bzbarsky) → needinfo?(michael)
(Assignee)

Comment 4

9 months ago
I'll look into it after I get the patches for bug 1365032 up (Which I hope will be soon). I'd rather not race my other major changes to that code :).
Assignee: nobody → michael
Flags: needinfo?(michael)
(Assignee)

Comment 5

9 months ago
Created attachment 8875460 [details] [diff] [review]
Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

If you're wondering why the WidgetEvent change is present in this patch, I honestly don't know either. Basically when I made the changes to the ipdl files, it randomly caused a warning which was made an error due to -WError start firing on that line of code. I had to add the explicit base class initialization to disable that warning.

No idea why that happened.

MozReview-Commit-ID: GLqs4YU0lbX
Attachment #8875460 - Flags: review?(bugs)
(Assignee)

Comment 6

9 months ago
Created attachment 8875461 [details] [diff] [review]
Part 2: Send DimensionInfo down to the content process synchronously when creating a new window

MozReview-Commit-ID: GlzJ491RLUE
Attachment #8875461 - Flags: review?(bugs)

Updated

9 months ago
Attachment #8875460 - Flags: review?(bugs) → review+

Updated

9 months ago
Attachment #8875461 - Flags: review?(bugs) → review+
(Assignee)

Comment 7

9 months ago
Created attachment 8875847 [details] [diff] [review]
Part 3: Add an ugly workaround for non-e10s windows

This is really gross. I don't know why but in non-e10s windows (which wasn't
affected by my patch) we don't seem to fire off a resize event if you resize
immediately after opening the window.

This changes the code to spin the event loop once before actually firing the
resize event. I would be really curious to know why the event wasn't being
fired.

MozReview-Commit-ID: 9l8tAWSo55P
Attachment #8875847 - Flags: review?(bugs)

Comment 8

9 months ago
I guess in non-e10s there might not be layout yet.

Comment 9

9 months ago
Comment on attachment 8875847 [details] [diff] [review]
Part 3: Add an ugly workaround for non-e10s windows

I thought this bug was about calling resizeBy immediately after opening, so how is this test testing that anymore?
Attachment #8875847 - Flags: review?(bugs) → review-
(Assignee)

Comment 10

9 months ago
Created attachment 8877204 [details] [diff] [review]
Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

I think I found a way to avoid needing to delay the call to resizeBy - this seems to be green on try.

MozReview-Commit-ID: GLqs4YU0lbX
Attachment #8877204 - Flags: review?(bugs)
(Assignee)

Updated

9 months ago
Attachment #8875460 - Attachment is obsolete: true
Attachment #8875847 - Attachment is obsolete: true
(Assignee)

Comment 11

9 months ago
Created attachment 8877304 [details] [diff] [review]
Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

Disabled the test on android, as testing resizeby on android doesn't really make sense.

MozReview-Commit-ID: GLqs4YU0lbX
Attachment #8877304 - Flags: review?(bugs)
(Assignee)

Updated

9 months ago
Attachment #8877204 - Attachment is obsolete: true
Attachment #8877204 - Flags: review?(bugs)

Comment 12

9 months ago
Comment on attachment 8877304 [details] [diff] [review]
Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

>+        // We resized synchronously. If this happened, we sometimes won't fire
>+        // an onresize event, so we resolve immediately.
Nit, there isn't such even as onresize. onresize is event handler name. Event is resize.

>+        if (popup.innerWidth == 550) {
>+          resolve(popup);
>+        } else {
>+          let onresize = () => {
hopefully this doesn't get mixed with onresize on global scope.
But renaming to popupresize might not be too bad idea.
Attachment #8877304 - Flags: review?(bugs) → review+

Comment 13

9 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f351fbc39a79
Part 1: Add a test for the behavior of window.resizeBy right after opening a new window, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd7bd8a399a
Part 2: Send DimensionInfo down to the content process synchronously when creating a new window, r=smaug

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f351fbc39a79
https://hg.mozilla.org/mozilla-central/rev/8dd7bd8a399a
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1373695
You need to log in before you can comment on or make changes to this bug.