Closed Bug 1341159 Opened 7 years ago Closed 7 years ago

Use commonLibRequire trick to reuse already loaded libraries

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Iteration:
54.3 - Mar 6
Tracking Status
firefox54 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(2 files)

Regression from bug 1309826. I removed commonLibRequire trick accidentally. We should put it back to avoid loading duplicated library instance such as React.
Iteration: --- → 54.2 - Feb 20
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
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)
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.
Iteration: 54.2 - Feb 20 → 54.3 - Mar 6
Flags: qe-verify? → qe-verify-
Jarda, this is a regression since I removed `commonLibRequire` accidentally. Have you run into this trouble before?
Flags: needinfo?(jsnajdr)
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)
But I do reproduce the issue. Nothing happens when I click on the filter buttons.
Attached patch react-dom.patchSplinter Review
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.)
(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.
I filed bug 1342297 to remove the React DOM patch.
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).
Flags: needinfo?(jsnajdr)
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+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b02bffae9133
Use commonLibRequire trick to reuse already loaded libraries r=Honza
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)
https://hg.mozilla.org/mozilla-central/rev/b02bffae9133
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.