Closed
Bug 1341159
Opened 7 years ago
Closed 7 years ago
Use commonLibRequire trick to reuse already loaded libraries
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox54 fixed)
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: rickychien, Assigned: rickychien)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor-reserve])
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
Honza
:
review+
|
Details |
1.04 KB,
patch
|
Details | Diff | Splinter Review |
Regression from bug 1309826. I removed commonLibRequire trick accidentally. We should put it back to avoid loading duplicated library instance such as React.
Updated•7 years ago
|
Blocks: netmonitor-html
Iteration: --- → 54.2 - Feb 20
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
I'm not sure why netmonitor is broken after adding `commonLibRequire: toolbox.browserRequire` back. Toggling filter buttons or network details panel button twice don't work as expected. However, everything looks good if we just remove `commonLibRequire: toolbox.browserRequire`. Honza, Alex, do you have any idea?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(odvarko)
Assignee | ||
Comment 3•7 years ago
|
||
Weird behavior is because we load all modules twice, so every action dispatch twice which is unexpected. We need to figure out why module will load twice after adding `commonLibRequire: toolbox.browserRequire` trick.
Updated•7 years ago
|
Iteration: 54.2 - Feb 20 → 54.3 - Mar 6
Updated•7 years ago
|
Flags: qe-verify? → qe-verify-
Assignee | ||
Comment 4•7 years ago
|
||
Jarda, this is a regression since I removed `commonLibRequire` accidentally. Have you run into this trouble before?
Flags: needinfo?(jsnajdr)
Comment 5•7 years ago
|
||
You must be confused by the browser toolbox which may be loading the netmon as well. As I do not see the xhtml not any module being loaded twice.
Flags: needinfo?(poirot.alex)
Comment 6•7 years ago
|
||
But I do reproduce the issue. Nothing happens when I click on the filter buttons.
Comment 7•7 years ago
|
||
I am attaching a patch that fixes the problem for me. @Greg: I didn't know about the monkey patch in react-dom, what is it for? Why it could break with the `commonLibRequire` flag in BrowserLoader? The problem is that events are fired twice (I tried 'click' and 'mousedown' events). Btw. I put a simple `console.log("ReactDev LOAD " + window.location.href);` in react-dev.js module (I am using --enable-debug-js-modules) and I can see that the module is loaded many times (for different iframes/URL). Looks like once per panel. No other panel, except of Net panel is using `commonLibRequire`. Honza
Flags: needinfo?(odvarko) → needinfo?(gtatum)
This patching of react-dom is pretty confusing in general I feel... Is there any other way to achieve the desired outcome? (It's fine to tweak it again here if we must, but it would be great to have a plan that makes this unnecessary.)
Comment 9•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > This patching of react-dom is pretty confusing in general I feel... Is > there any other way to achieve the desired outcome? (It's fine to tweak it > again here if we must, but it would be great to have a plan that makes this > unnecessary.) One other solution mentioned was modifying platform code: https://bugzilla.mozilla.org/show_bug.cgi?id=1245921#c69 Though Greg ruled went with the patching because he wanted to avoid breaking tests. The desire outcome is described from comments 59 to 68 of that bug.
Comment 10•7 years ago
|
||
There was a WIP platform patch: https://gist.github.com/gregtatum/352af65d739d8895dedc2a8d8ea6016d
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
I filed bug 1342297 to remove the React DOM patch.
Assignee | ||
Comment 13•7 years ago
|
||
Thank you ntim! My patch has updated and merged the react-dom.patch from comment 7. It fixed the problem for me. It seems that we can land this workaround patch first, and then deal with this root cause from platform (bug 1342297).
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jsnajdr)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8839310 [details] Bug 1341159 - Use commonLibRequire trick to reuse already loaded libraries https://reviewboard.mozilla.org/r/113976/#review116652 Good, let's remove the hack in react-dev.js as soon as bug 1342297 is fixed. Thanks Ricky! Honza
Attachment #8839310 -
Flags: review?(odvarko) → review+
Comment 15•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b02bffae9133 Use commonLibRequire trick to reuse already loaded libraries r=Honza
Comment 16•7 years ago
|
||
The issue was with React's synthetic event dispatch system. It captures all events at the root element, and then dispatches them out to the individual components using only React. The problem is that the events are crossing over iframes in ways that the normal web can't do it, so React is dispatching the event twice. So the true fix is to fix the platform as outlined in the toolbar bug. I took a stab at it, but it quickly got gnarly and broke a lot of things. The best solution is definitely to fix the platform to conform to the way the greater web works, so that monkey patch doesn't need to exist.
Flags: needinfo?(gtatum)
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b02bffae9133
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•