Closed Bug 1044586 Opened 5 years ago Closed 3 years ago

F5 key breaks about:preferences

Categories

(Core :: DOM: Events, defect)

34 Branch
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: vvdun18, Assigned: Gijs)

References

()

Details

(Whiteboard: [sf-hackweek])

Attachments

(6 files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140727030204

Steps to reproduce:

1. Open in-content Options (about:preferences) 
2. Hold down the F5 for a few seconds
3. Finish! Options displays a blank page.


Actual results:

Options are not displayed correctly


Expected results:

Settings should be displayed correctly
Severity: normal → minor
Component: Untriaged → Preferences
I would not expect holding down F5 to be normal behavior - a keypress would do.  

However, I did replicate this issue in 34.0a1 (2014-07-28).  

Pressing F5 on that blank screen, however, brings about:preferences back as expected.
Jared, I don't think this needs to block shipping in-content prefs as-is, but I wondered if you have an offhand clue on what might cause this and/or what we could do about it, if anything - if it's low-hanging fruit and/or points to a more serious issue, it might be worth looking into.
Flags: needinfo?(jaws)
It looks like it is a race condition with how the preferences are loaded. We call init_all on DOMContentLoaded, so if there is an 'unload' followed by a 'DOMContentLoaded' (not sure if that is possible) could maybe cause this.

See attached screenshot showing how references are undefined.
Flags: needinfo?(jaws)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 1097393
Blocks: 718011
Points: --- → 3
OS: Windows 8.1 → All
Priority: -- → P4
Hardware: x86_64 → All
Whiteboard: [sf-hackweek]
Blocks: 1271779
Make no longer block 718011 since it is not about moving into in-content and already being checked by 1271779.
No longer blocks: 718011
I take a look at this bug, it looks like the problem here is the race condition between different modules (gSubDialog, gMainPane, etc) and preferences.js.

In preferences.js, those module will be access directly after DOMContentLoaded.

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#48-56

If those script did not load properly before the DOMContentLoaded event triggered, it will cause an undefined error just like the screenshot Jared has provided in comment 3.
Assignee: nobody → jyeh
After some investigation and a former offline discussion with Fischer, a simple solution would be initializing after the `load` event instead of the `DOMContentLoaded` event to make sure we can access those modules after they are fully loaded.

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#40

However, in this case we will break several tests since the order of the events got triggered has changed. Some events are expected to be triggered after the `DOMContentLoaded` event may now triggered after the `load` event.

Some tests are easy to fix, but few of them are quite complicate which I am still looking at it.


Another solution might look like workaround which is using try and catch when accessing those modules in 

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#48-56

and simply do a setTimeout to recursively call the `init_all` again when we caught an undefined error.


@Jared, which solution in the above do you think is more appropriate? or maybe you can give some advice here :)

Thanks!
Flags: needinfo?(jaws)
(In reply to Joseph Yeh [:joseph] from comment #7)
> If those script did not load properly before the DOMContentLoaded event
> triggered

This shouldn't be possible. DOMContentLoaded implies all of the DOM has loaded. Scripts are expected to load and execute before that happens, because the scripts might modify the DOM - they should be read, parsed and executed before the parser continues loading the rest of the DOM.

Have you actually verified that this diagnosis is correct? How?
(In reply to :Gijs Kruitbosch from comment #9)
> This shouldn't be possible. DOMContentLoaded implies all of the DOM has
> loaded. Scripts are expected to load and execute before that happens,
> because the scripts might modify the DOM - they should be read, parsed and
> executed before the parser continues loading the rest of the DOM.

That's what I expected too.

After printing some logs and comparing between multiple reloads, I finally realize the problem here.

As the screenshot in the attachment shows, the logs on the left is by doing only one reload. Each modules load properly before the `DOMContentLoaded` event triggered. The logs on the right side is the main issue described in this bug by doing two reloads very closely. 

You can see that there are two `DOMContentLoaded` events in the logs which triggered by two reloads respectively. Unfortunately, as the blue box points out, the first `DOMContentLoaded` event happens right after the `unload` event which leads to an unexpected initialization for the preferences. And the second `DOMContentLoaded` event will not do the initialization like we expected because the event listener has already been removed by the unexpected initialization.

So I think the possible solution here is not removing the event listener until we make sure all modules are loaded properly. And also discard the initialization triggered by the unexpected `DOMContentLoaded` event.
Flags: needinfo?(jaws)
(In reply to Joseph Yeh [:joseph] from comment #10)
> Created attachment 8802448 [details]
> compare-logs-with-multiple-reloads.png
> 
> (In reply to :Gijs Kruitbosch from comment #9)
> > This shouldn't be possible. DOMContentLoaded implies all of the DOM has
> > loaded. Scripts are expected to load and execute before that happens,
> > because the scripts might modify the DOM - they should be read, parsed and
> > executed before the parser continues loading the rest of the DOM.
> 
> That's what I expected too.
> 
> After printing some logs and comparing between multiple reloads, I finally
> realize the problem here.
> 
> As the screenshot in the attachment shows, the logs on the left is by doing
> only one reload. Each modules load properly before the `DOMContentLoaded`
> event triggered. The logs on the right side is the main issue described in
> this bug by doing two reloads very closely. 
> 
> You can see that there are two `DOMContentLoaded` events in the logs which
> triggered by two reloads respectively. Unfortunately, as the blue box points
> out, the first `DOMContentLoaded` event happens right after the `unload`
> event which leads to an unexpected initialization for the preferences. And
> the second `DOMContentLoaded` event will not do the initialization like we
> expected because the event listener has already been removed by the
> unexpected initialization.
> 
> So I think the possible solution here is not removing the event listener
> until we make sure all modules are loaded properly. And also discard the
> initialization triggered by the unexpected `DOMContentLoaded` event.

This still doesn't make an awful lot of sense to me. Why is DOMContentLoaded firing? And is that on the old document (on which unload fired) or on the new document, or on a subdocument (like a frame)? If it's on the old document, why/how is that affecting the new document (post-reload)? What patch is generating these logs?

If gecko/DOM is doing stupid things here, I'd rather fix that than work around it on the frontend side.
(In reply to :Gijs Kruitbosch from comment #11)
> This still doesn't make an awful lot of sense to me. Why is DOMContentLoaded
> firing? And is that on the old document (on which unload fired) or on the
> new document, or on a subdocument (like a frame)? If it's on the old
> document, why/how is that affecting the new document (post-reload)? What
> patch is generating these logs?

The first DOMContentLoaded event doesn't make sense to me either. I can provide the patch that I use to generate these logs.

> If gecko/DOM is doing stupid things here, I'd rather fix that than work
> around it on the frontend side.

Yes, I agree with you. If this is something gecko should handle, we should let them fix it.
Attached patch Logging patch v2Splinter Review
So... this is interesting.

I adapted Joseph's patch to set an expando on the document as preferences.xul first gets loaded, and to log that expando with all the other logs, including from the DCL (DOMContentLoaded) event handler.

STR:

1. apply patch on opt build (haven't tested debug, but because it's slower you might struggle to reproduce)
2. open the preferences/options
3. hold down accel-r / f5

ER:
the expando on |document| in the scope of the DOMContentLoaded event listener matches the expando on event.target of the DCL event listener at all times (no subframes get loaded, or the expando would be undefined, which would at least make some sense)

AR:
There are mismatches. DCL fires (if you don't unbind it, fires a number of times!) for documents that are no longer the current |document| in that |window|.



The DCL listener is attached to |window|. I checked, and the whole problem goes away if you just attach the listener to |document| instead. So that's the trivial workaround to fix the preferences. However, in my limited understanding of DOM / the web it should not be possible for DCL to fire on a toplevel document that is not the current document in a |window|.

In theory this could be a XUL quirk in which case it might make sense to not fix it. But from a quick look at the code, DCL is fired from general document code and not different for XUL (again, I could be wrong). If so, I'm worried that this problem could in principle affect web pages, which would (again, assuming my understand isn't broken somehow) be potentially problematic.

baku, is there any chance you know enough of the DOM innards here to help out, or can you nominate someone else?

From what I can tell
Flags: needinfo?(amarchesini)
(In reply to :Gijs Kruitbosch from comment #13)
> From what I can tell

eh, ignore that - silly disappearing scrollbars, I didn't realize there was trailing text still.
Assign to Fischer for further follow up.
Assignee: jyeh → fliu
I don't know enough about XUL to answer. smaug, maybe? See comment 13.
Flags: needinfo?(amarchesini) → needinfo?(bugs)
Why DOMContentLoaded wouldn't fire on non-current-document? DCL is after all about document load, unlike load event.
What is surprising is why a listener in the window gets triggered.
Does nsDocument::PreHandleEvent end up propagating the event to window even if GetInnerWindow() isn't the current inner window in the outer window?
Could someone check that?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #17)
> Why DOMContentLoaded wouldn't fire on non-current-document? DCL is after all
> about document load, unlike load event.
> What is surprising is why a listener in the window gets triggered.
> Does nsDocument::PreHandleEvent end up propagating the event to window even
> if GetInnerWindow() isn't the current inner window in the outer window?
> Could someone check that?

Are you just asking whether this code:

https://dxr.mozilla.org/mozilla-central/rev/e3279760cd977aac30bd9e8032d3ee71f55d2a67/dom/base/nsDocument.cpp#7605-7620

does any checks on this? It seems like a very straightforward method that indeed doesn't do any checks of that sort (also not for any other events, for that matter, except that |load| events don't ever hit the window). Did I misunderstand the question?
Flags: needinfo?(bugs)
yeah, that code. It uses outer window (GetWindow()), but it could first check if document's inner window is the current window, maybe.
Flags: needinfo?(bugs)
This seems to have gotten lost. Self-needinfo to get back to this.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8815818 [details]
Bug 1044586 - fix propagating document events to the window when the inner window has changed,

https://reviewboard.mozilla.org/r/96598/#review96820

Tiny bit scary, but don't have better ideas.
Attachment #8815818 - Flags: review?(bugs) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/512d5944a602
fix propagating document events to the window when the inner window has changed, r=smaug
(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8815818 [details]
> Bug 1044586 - fix propagating document events to the window when the inner
> window has changed,
> 
> https://reviewboard.mozilla.org/r/96598/#review96820
> 
> Tiny bit scary, but don't have better ideas.

Yeah - I thought of trying to write a test and gave up pretty quickly... Try looked green (besides random splattering of intermittent orange), so I landed.
https://hg.mozilla.org/mozilla-central/rev/512d5944a602
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Reassign to :Gijs because this bug is fixed by him.
Assignee: fliu → gijskruitbosch+bugs
Severity: minor → normal
Component: Preferences → DOM: Events
Flags: qe-verify?
Priority: P4 → --
Product: Firefox → Core
Target Milestone: Firefox 53 → mozilla53
Depends on: 1348623
Duplicate of this bug: 1317798
You need to log in before you can comment on or make changes to this bug.