Bugzilla – Full Text Bug Listing
| Summary: | Async initialization fails in Firefox with chrome privileges | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Mihai Sucan <mihai.sucan> |
| Component: | Editor | Assignee: | Felipe Heidrich <eclipse.felipe> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | eclipse.felipe, mihai.sucan |
| Version: | unspecified | ||
| Target Milestone: | 0.4 M1 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
|
Description
Mihai Sucan
Proposed fix: https://github.com/mihaisucan/orion.client/tree/bug-363945 Changes: 1. I set useCapture=true only for Firefox. This alone is sufficient for Orion to start in Firefox with chrome privileges (in our developer tools integration code). 2. I remove the load event listeners once they fire. Having them there at all times is prone to errors. For example _loadHandler is called twice (once for about:blank, and the second time when document.write() happens in _createView). 3. the addHandler(evName, this._foo = function() {}) trick doesn't work. Later when you do removeHandler(evName, this._foo) you don't actually remove the event handler. This is because this._foo gets a different copy of the function() {} than addHandler(). Technically the two copies are not the same - the removeHandler() calls never did what they were supposed to. Tested with Firefox, Chrome and Opera. Things didn't break. Please review and let me know if further changes are needed before this fix can land in Orion. Thank you very much! I'm okay with (1) and (3). But not with change (2), the reason is that it breaks (I suspect) the case where editor is removed from the tree and then re-inserted. during remove the view handles the unload event (and destroy the internal elements) but when it is re-inserted it never get the load (which would recreate the internals). Besides, the load handler should detect that the work has already been done and run away without hurting anything in the second call. Right ? As for (3), interesting scenario, I will need to check that we don't do that mistake on other places. (In reply to comment #2) > I'm okay with (1) and (3). > > But not with change (2), the reason is that it breaks (I suspect) the case > where editor is removed from the tree and then re-inserted. > during remove the view handles the unload event (and destroy the internal > elements) but when it is re-inserted it never get the load (which would > recreate the internals). (2) doesn't break that. I know of that case, and I tested it. The load event listeners are added as needed when the view is recreated. Please test. > Besides, the load handler should detect that the work has already been done and > run away without hurting anything in the second call. Right ? True, but again, that doesn't mean it will work properly in the future. (code is prone to errors) > As for (3), interesting scenario, I will need to check that we don't do that > mistake on other places. Please do. Thank you! (In reply to comment #3) > (In reply to comment #2) > (2) doesn't break that. I know of that case, and I tested it. The load event > listeners are added as needed when the view is recreated. Please test. Okay, yesterday I only read the code. This morning I did test it. The change does break the case as I expected it. In the demo.js replace the empty test method by: var el1, el2; function test() { if (!el1) { el1 = document.getElementById("divParent"); el2 = el1.parentNode; } if (el1.parentNode === el2) { el2.removeChild(el1); } else {{ el2.appendChild(el1); }} } open http://localhost:8080/examples/textview/demo.html click on Create Java (or Create JavaScript) sroll down a bit, select some text click Test - the entire text view disappears click Test again - the entire view reappears (preserving the srolling and selection) apply the patch in comment 1 repeat the above the text view will not reappear - The view depends on the _loadHandler to be called to rebuild to DOM. If I comment the lines that remove the _loadHandler after the first time it is invoked then everything works again. Please, try the scenario I described above. > > True, but again, that doesn't mean it will work properly in the future. (code > is prone to errors) > I understand, but do you have a case there it is failing ? (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > (2) doesn't break that. I know of that case, and I tested it. The load event > > listeners are added as needed when the view is recreated. Please test. > > Okay, yesterday I only read the code. This morning I did test it. The change > does break the case as I expected it. > > In the demo.js replace the empty test method by: > var el1, el2; > function test() { > if (!el1) { > el1 = document.getElementById("divParent"); > el2 = el1.parentNode; > } > if (el1.parentNode === el2) { > el2.removeChild(el1); > } else {{ > el2.appendChild(el1); > }} > } > > open http://localhost:8080/examples/textview/demo.html > click on Create Java (or Create JavaScript) > sroll down a bit, select some text > click Test - the entire text view disappears > click Test again - the entire view reappears (preserving the srolling and > selection) > > apply the patch in comment 1 > repeat the above > the text view will not reappear > > - The view depends on the _loadHandler to be called to rebuild to DOM. > If I comment the lines that remove the _loadHandler after the first time it is > invoked then everything works again. > > Please, try the scenario I described above. I can repro the problem. Thanks for the STR. My STR consistent of opening the Web Console and doing: $("divParent").style.display = 'none' then 'block'. That case works even with the patch I provided. (there was a bug open in this bugzilla about this case) I didn't know that Orion is supposed to not break after re-appending. > > True, but again, that doesn't mean it will work properly in the future. (code > > is prone to errors) > > > > I understand, but do you have a case there it is failing ? Given the situation, then, don't remove the event listeners. Mainly I am interested of the _loadHandler to have useCapture=true on Firefox, such that Orion doesn't fail for us with chrome privileges enabled. Thanks for your patience and code reviews! Please also note there's a typo in UndoStack, _onDestroy:
this.view.removeEventListener("Destroy", this._listnener.onDestroy);
that should be _listener.
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=6af407960d3ada52dc93c7cdfd4ddb0d4b14e95a http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=1a1caa66cddbd90c9e900c9ac815d676ce260446 Please test, thank you for the patch. (In reply to comment #6) > Please also note there's a typo in UndoStack, _onDestroy: > > this.view.removeEventListener("Destroy", this._listnener.onDestroy); > > that should be _listener. Fixed. Thank you. Mihai feel free to push this type of changes yourself. (In reply to comment #8) > (In reply to comment #6) > > Please also note there's a typo in UndoStack, _onDestroy: > > > > this.view.removeEventListener("Destroy", this._listnener.onDestroy); > > > > that should be _listener. > > Fixed. Thank you. Thank you! > Mihai feel free to push this type of changes yourself. Will do! |